0

Here is the problem, when it encounters fractions like: 300/10 instead of giving a result of "30" the following code gives me: 1/0 

$tokens = explode('/', $value);
while ($tokens[0] % 10 == 0) {
   $tokens[0] = $tokens[0] / 10;
   $tokens[1] = $tokens[1] / 10;
}
if ($tokens[1] == 1) {
   return $tokens[0].' s';
} else {
  return '1/'.floor(1/($tokens[0]/$tokens[1])).' s';
   // return $tokens[0].'/'.$tokens[1].' s';
}

thanks

htfree
  • 331
  • 1
  • 15
  • 1
    On the first while iteration `$tokens[0] = 30 (300/10)`,`$tokens[1] = 1 (10/10)`, on the second while iteration `$tokens[0] = 3 (30/10)`,`$tokens[1] = 0.1 (1/10)`, `else` case is working. – u_mulder Oct 08 '14 at 19:47
  • because you reduced $tokens[1] down to 0.1 – tbddeveloper Oct 08 '14 at 19:47
  • U_mulder and Hammer thanks for comments but please specify exactly how you would propose modifying my code to best cover such cases and make it as an answer to the question so that I can 'click' to accept your answer as the best solution. thanks – htfree Oct 08 '14 at 20:15
  • In other words my own quick fix was just change % 10 to % 100 but maybe that breaks something else another case/fraction style and is not best fix? I'm literally not feeling well right now so not thinking very good. – htfree Oct 08 '14 at 20:19

2 Answers2

0

You should change the line while($tokens[0] % 10 === 0 && $tokens[1] % 10 === 0) { to while($tokens[0] % 10 === 0 && $tokens[1] % 10 === 0) {.

And the line return '1/'.floor(1/($tokens[0]/$tokens[1])).' s'; is not reliable.

If you want to reduce fractions, try this function:

function reduceFraction($fraction) {
    sscanf($fraction, '%d/%d %s', $numerator, $denominator, $junk);

    // TODO: validation

    if( $denominator === null ) {
        return (string)$numerator;
    }

    if( $numerator === $denominator ) {
        return 1;
    }

    $max = max(array($numerator, $denominator));
    for($i = 1; $i < $max; ++$i) {
        if( $denominator % $i === 0 && $numerator % $i === 0) {
            $common = $i;
        }
    }

    if( $denominator === $common ) {
        return (string)($numerator / $common);
    }

    return ($numerator / $common) . '/' . ($denominator / $common);
}

You could use it like this: reduceFraction('300/10') . ' s';

It's also possible to generalize more the function for chained fractions (eg: '300/100/10'). I can send an implementation of it if you wish.

Pedro Amaral Couto
  • 2,056
  • 1
  • 13
  • 15
  • Thanks, I tried your modification and seems to do the same thing as my fix of changing % 10 to "% 100" so can you tell me why the "while ($tokens[0] % 10 == 0 && $tokens[1] % 10 ==0)" would be better to use than just "while ($tokens[0] % 100 == 0)" since both methods seem to work ok. Then I will click to accept answer, thanks – htfree Oct 08 '14 at 22:14
  • I added another answer explaining why "while ($tokens[0] % 10 == 0 && $tokens[1] % 10 ==0)" is better than "while ($tokens[0] % 100 == 0)". They both return the same string when you use "300/10" as an argument ("30 s"), but they return different strings when you use "3000/10" as an argument ("1/0 s" and "30 s"). The later (my suggestion) is the one that returns a correct value, but I'd suggest you to use the "reduceFraction" implementation because it works for any fraction n/m. – Pedro Amaral Couto Oct 10 '14 at 21:58
0

tell me why the "while ($tokens[0] % 10 == 0 && $tokens[1] % 10 ==0)" would be better to use than just "while ($tokens[0] % 100 == 0)" since both methods seem to work ok

If you try to use the string "3000/10" as an argument for each implementation, the one with while ($tokens[0] % 10 == 0 && $tokens[1] % 10 ==0) will return 300 s, and the other with while ($tokens[0] % 100 == 0) will return 1/0 s.

If you use the while ($tokens[0] % 100 == 0) method, the loop iterations are:

  1. $tokens[0] = 3000 / 10 = 300; $tokens[1] = 10 / 10 = 10;
  2. $tokens[0] = 30 / 10 = 30; $tokens[1] = 10 / 1 = .1; Stopped because 30 % 100 != 0. Since the $token[1] is not 1, it does not return "30 s". 1/30 is less than zero (0.0333...), thus floor(1/30) = 0. That's why it returns "1/0 s".

If you use the while ($tokens[0] % 10 == 0 && $tokens[1] % 10 == 0) method, the loop iterations are:

  1. $tokens[0] = 3000 / 10 = 300; $tokens[1] = 10 / 10 = 1; Stopped because 1 % 10 != 0. Since the $token[1] is not 1, it returns "30 s".

It is better because it will work with more inputs.

But I recommend you to use the "reduceFraction" function that I implemented.

It uses the maximum common denominator technique to reduce functions.

  • echo reduceFraction('3000/10'); outputs "300".
  • echo reduceFraction('300/10'); outputs "30".
  • echo reduceFraction('30/10'); outputs "3".
  • echo reduceFraction('3/10'); outputs "3/10".
  • echo reduceFraction('3/3'); outputs "1".
  • echo reduceFraction('222/444'); outputs "1/2".
  • echo reduceFraction('444/222'); outputs "2".
Pedro Amaral Couto
  • 2,056
  • 1
  • 13
  • 15