current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 0 down vote favorite

I'm trying to solve one simple task and on first look it's working.
Any advice on how I can optimize this code will be appreciated.
A quick description of the task would be:
You have a file from which you read the integers.

The first line has two values:

  1. The array width
  2. The number of test cases

The second line has actual array values.
After that, you have \$N\$ rows all containing the array range which should be checked.

\$N =\$ the number of test cases.
Here's a link to my code

<?php
$handle = fopen("php://stdin", "r");
$getFirstRow = fgets($handle);
$firstrow = explode(" ", $getFirstRow);
$firstrow1 = intval($firstrow[0]);
$firstrow2 = intval($firstrow[1]);

$getString = fgets($handle);
for ($z=0;$z<$firstrow2;$z++)
{
$getRange = fgets($handle);
$getRagearr = explode(" ", $getRange);
$i=intval($getRagearr[0]);
$j=intval($getRagearr[1]);
$arrResult = array();
$lane = explode(" ", $getString);
for ($p=$i;$p<=$j;$p++)
{
array_push($arrResult, $lane[$p]);
}
$result = min($arrResult);
echo $result.PHP_EOL;
}
share|improve this question
    
Even if you provide a link to the code in a third party site, you must still include the relevant parts to be reviewed in the question itself. Please update your post to include the source code to be reviewed. –  glampert May 27 at 15:23
    
Ok sure, I will keep this in mind for future! I added the code into post –  tslid May 28 at 10:15
    
One tip, give your variables handy names, like your $firstrow2 should be $maxTestCases or something in that way. –  MHakvoort May 28 at 13:02

2 Answers 2

up vote 4 down vote accepted

You should indent your code properly to make it more readable.

Posting PHP on JSFiddle is apocalyptic insanity a bad idea. JSFiddle is for JavaScript.
Use IDEOne for PHP Instead.

Your use of whitespace could be improved.

 for ($z=0;$z<$firstrow2;$z++)

into: for ($z = 0; $z < $firstrow2; $z++)

Your variable naming is shortened, sometimes confusing, and flat out bad practice in places.
Transform them from lowercase to camelCase.

You don't even use $firstrow1, remove it.

$getRagearr sounds like a pirate themed narcotic. Perhaps $getRangeArray would be much clearer.

$j=intval($getRagearr[1]): what's wrong with good ole' fashioned int()?'

This is the result of all my suggestions:

$handle = fopen("php://stdin", "r");
$getFirstRow = fgets($handle);
$firstRowArray = explode(" ", $getFirstRow);
$numOfTestCases = int($firstRow[1]);

$getString = fgets($handle);
for ($i = 0; $i < $numOfTestCases; $i++) {
    $getRange = fgets($handle);
    $getRangeArray = explode(" ", $getRange);
    $arrayWidth = int($getRangeArray[0]);
    $arrayLength = int($getRangeArray[1]);
    $arrayResult = array();
    $lane = explode(" ", $getString);
    for ($currentPos = $arrayWidth; $currentPos <= $arrayLength; $currentPos++) {
        array_push($arrayResult, $lane[$currentPos]);
    }
    $result = min($arrayResult);
    echo $result.PHP_EOL;
}

Other than that, your code looks well-structured.

share|improve this answer
up vote 2 down vote

You read a line and split it by spaces multiple times. To avoid such repeated logic, I suggest creating a function:

function read_line_as_array($fh) {
    return explode(" ", fgets($fh));
}

And instead of getting an array of values from a line, and then assigning to variables by indexes, you can benefit from array unpacking, like this:

$input = fopen("php://stdin", "r");
list(, $testCount) = read_line_as_array($input);

$arr = read_line_as_array($input);
for ($i = 0; $i < $testCount; $i++)
{
    list($start, $end) = read_line_as_array($input);
    $min = $arr[$start];
    for ($j = $start + 1; $j <= $end; $j++)
    {
        if ($arr[$j] < $min) {
            $min = $arr[$j];
        }
    }
    echo $min . PHP_EOL;
}

Notice how I read into the $start and $end variables directly, no need for temporary variables to store the line.

I also dropped all the intval calls, simply because my test cases work just fine without it. If it turns out to be necessary, you can simply improve on read_line_as_array to return proper integers, and then the rest of the code will work just fine.

Another improvement I did is calculating the $min in a single pass and without a temporary array: the original code makes one pass to create a temporary array, and then calls min(...) on it, which is a second pass.

Last but not least, I cleaned up the variable names. $i and $j could be better, but they are acceptable as simple loop variables. (The $i and $j had a different meaning in the original code, and not used as loop variables, in which case they were not acceptable names.)

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.