PHP function to compare two Roman numerals This was for an assignment. I had to write a function to compare two roman numerals. This had to return true only if the second number was larger than the first and had to be done without converting the letters into integers. Also prefix subtraction didn't apply so all the letters came in descending order.

Here is what I came up with:

&lt;?php

function x_smaller_than_y($x_num,$y_num) {

//Create array with ordered list of roman numerals. 
$evaluation= array("M","D","C","L","X","V","I");

//Splits submitted numbers into arrays
$x_num_arr = str_split($x_num);
$y_num_arr = str_split($y_num);

//Loops through each roman numeral in the given list
for ($evaluate = 0; $evaluate&lt;7;$evaluate++)
  {  

     //For every letter in the numerals' list loops through 
     //every letter in the given numbers looking for a match.
     $counter = 0;
     while ($counter &lt; ((strlen($x_num)))&amp;&amp; $counter &lt; ((strlen($y_num))))
       {

       //If in any specific position x=y, it simply ignores it and moves forward.
       if ($x_num_arr[$counter]==$y_num_arr[$counter])
           {$counter++;}

       //If in any given position the letter in Y is higher in the list than  
       //the letter in X then Y is larger than X and the function is true. 
       //"break" ends the while loop and "$evaluate=8" ends the for loop.
       elseif(($x_num_arr[$counter]!=$evaluation[$evaluate])&amp;&amp;($y_num_arr[$counter]==$evaluation[$evaluate]))
           {return true;
            $evaluate=8;
            break;}

       //Same as above but this time the letter in X is before the letter 
       //in Y so the function is false.
       elseif(($x_num_arr[$counter]==$evaluation[$evaluate])&amp;&amp;( $y_num_arr[$counter]!=$evaluation[$evaluate]))
           {return false;
            $evaluate=8;
            break;}    

       //If none of the above cases apply simply move to the next letter.
       else {$counter++;}
        }
    }

//Since the number of iterations in the while loop is determined by the 
//shorter number there can still be a case in which 2 numbers are exactly 
//the same up to a point but one is simply longer than the other, 
//eg. MXVI and MXVII.
//When this happens the for loop will have run through its natural 
//course, $evaluate will be equal to 7, and the code below will determine 
//the larger number by looking at their lenght. 
if ($evaluate&lt;8&amp;&amp;(strlen($x_num) &lt; strlen($y_num)))
{return true;}

elseif ($evaluate&lt;8&amp;&amp;(strlen($x_num) &gt; strlen($y_num)))
{return false;}

}


How could have I made it better?
It gives wrong results for 9 (IX) and 5 (V).

Some notes about the code:


In the following block the $evaluate = 8; and the break; statements are unnecessary, they never run because of the return statement.

{
    return true;
    $evaluate = 8;
    break;
}

Instead of commenting name the variable to what the comment says. 

//Create array with ordered list of roman numerals. 
$evaluation= array("M","D","C","L","X","V","I");


It could be $numerals or $numerals_in_order, for example.
7 and 8 are a magic numbers. They should be named constants or computed values. Do they come from the size of the array? If they do make it clear, it would improve readability.
while ($counter &lt; ((strlen($x_num)))&amp;&amp; $counter &lt; ((strlen($y_num))))


This could be simplified to the following:

$size = min(strlen($x_num), strlen($y_num));
while ($counter &lt; $size)

Instead of big if-elseif structures you could use continue and simple ifs since some of them returns the function if the condition is true, so the remaining code won't run. The last else also would be unnecessary.

$size_min = min(strlen($x_num), strlen($y_num));
while ($counter &lt; $size_min) {
    if ($x_num_arr[$counter] == $y_num_arr[$counter]) {
        $counter++;
        continue;
    }

    if(($x_num_arr[$counter]!=$evaluation[$evaluate])&amp;&amp;($y_num_arr[$counter]==$evaluation[$evaluate])) { 
        return true;
    }

    if(($x_num_arr[$counter]==$evaluation[$evaluate])&amp;&amp;( $y_num_arr[$counter]!=$evaluation[$evaluate])) {
        return false;
    }    

    $counter++;
}

Inside the for loop the $evaluation array is always indexed with $evaluate. You should create a local variable for this.
The same is true for $y_num_arr[$counter] and $x_num_arr[$counter] inside the while loop. (They could be $current_x and $current_y).
It might not be an issue (depending on the specification) but the function currently accepts invalid Roman numerals too (like IIX).
At the end of the function it returns with implicit NULL if both conditions are false in the following snippet:

if ($evaluate&lt;8&amp;&amp;(strlen($x_num) &lt; strlen($y_num)))
{return true;}

elseif ($evaluate&lt;8&amp;&amp;(strlen($x_num) &gt; strlen($y_num)))
{return false;}


You should make this explicit (add a return false as the last statement) or use an assert call if it should never happen.
You could easily write some test code to check that the comparison is fine for every possible Roman numeral.

for ($i = 1; $i &lt;= $max; $i++) {
    for ($j = 1; $j &lt;= $max; $j++) {
        $i_r = numberToRoman($i);
        $j_r = numberToRoman($j);
        $smaller = x_smaller_than_y($i_r, $j_r);
        if ($i &gt;= $j &amp;&amp; $smaller) {
            echo "error: $i, $j, $i_r, $j_r\n";
        }
    }
}


If I'm right $max should be 3888 and it finishes in a few minutes. You can find a numberToRoman function here, for example..