2
\$\begingroup\$

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;
}
\$\endgroup\$
3
  • \$\begingroup\$ 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. \$\endgroup\$
    – glampert
    May 27 '15 at 15:23
  • \$\begingroup\$ Ok sure, I will keep this in mind for future! I added the code into post \$\endgroup\$
    – tslid
    May 28 '15 at 10:15
  • \$\begingroup\$ One tip, give your variables handy names, like your $firstrow2 should be $maxTestCases or something in that way. \$\endgroup\$
    – MHakvoort
    May 28 '15 at 13:02
5
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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.)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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