Convert Repeating Decimal to Fraction

Question

My code converts repeating decimals to fractions. It works but I am interested in feedback on how I could improve and/or simplify it?

jsFiddle: http://jsfiddle.net/tfgTu/

<script>
// Repeating decimal
// 21.83(35745857)
var decimal = 21.8335745857

// Total length of the decimal
var decimalLength = 10;

// Length of the decimal before the repeating part
var decimalBeforeRepeatLength = 2;


// 10 to the total length of the decimal
var power1 = Math.pow(10, decimalLength);

// 10 to the length of the decimal before the repeating part
var power2 = Math.pow(10, decimalBeforeRepeatLength);

var finalPower = power1 - power2;


var decimal1 = decimal * power1;

var decimal2 = parseInt(decimal * power2);

var finalDecimal = decimal1 - decimal2;

var output = simplifyFraction(0, finalDecimal, finalPower);



document.write(output[1] + " / " + output[2]);




// Finds the greatest common divisor of 2 non-zero integers
// a = integer 1
// b = integer 2
function greatestCommonDivisor(a, b) {
    if (b === 0) {
        return a;
    }

    return greatestCommonDivisor(b, (a % b));
}


// Simplifies all forms of fractions
// Does not change form of fraction
// i = number
// n = numerator
// d = denominator
function simplifyFraction(i, n, d) {
    var gcd = greatestCommonDivisor(n, d);

    if (Math.abs(i) > 0 && n >= d) { // If mixed number AND improper fraction, assume the user wants a simplified mixed number

        if (i < 0) {
            n *= -1;    
        }

        var temp = improperFractionToMixedNumber(n, d);

        return [temp[0] + i, temp[1] / gcd, temp[2] / gcd];
    }

    return [i, n / gcd, d / gcd];
}
</script>

Answer 1

1. While the general code quality is good, the contract should be formalized by adding "use strict";
2. Even though var is used, everything is still in the global namespace, thereby polluting it and risking name collisions. Moving things into a namespace also means some other parts of the code can/should be re-arranged.
3. There's ideally no need to manually specify both the number, and how many digits there are after the ..
4. I honestly fail to understand the problem the function solves, so it's unclear to me if var decimalBeforeRepeatLength = 2 can also be derived automatically from the input, or if it needs to be a separate input parameter.
5. There's no radix parameter to parseInt(). Without it, the "correct" behaviour is not guaranteed, so you really should specify it. See Mozilla's documentation for details.
6. It's unclear to me why parseInt() is used, why not just Math.floor()?
7. Separating the output from the functionality would be a nice improvement.
8. improperFractionToMixedNumber isn't included in the actual code you posted, so I can't really comment on that, nor test that the code gets the same results before and after my changes.
9. Since i is hardcoded to 0, most of the original simplifyFraction() can actually be removed all together, but I'm guessing making i a parameter would be the solution instead.
10. Why does simplifyFraction() return i when it's an unmodified input parameter?
11. With some of the changes I've done, a few variables should probably have their names changed. I'll leave that as an exercise to the reader.

Code below incorporating the above changes. I've removed all original comments and added my own, to make it easier to spot the differences.

// Isolate variables and functions from the global scope.
// The namespace should of course not actually be named 'namespace'.
var simplifyFraction = function(decimal, decimalBeforeRepeatLength) {
   // Improve the code quality with strict mode.
  "use strict";

  // Figure out decimal length automatically.
  var indexOfDot = decimal.toString().indexOf('.');
  var decimalLength = decimal.toString().length - indexOfDot - 1;

  var power1 = Math.pow(10, decimalLength);
  var power2 = Math.pow(10, decimalBeforeRepeatLength);
  var finalPower = power1 - power2;
  var decimal1 = decimal * power1;
  // Explicitly set a radix of '10' to avoid unpredicatable results.
  var decimal2 = parseInt(decimal * power2, 10);
  // Preferred option.
  decimal2 = Math.floor(decimal * power2);
  var finalDecimal = decimal1 - decimal2;
  var i = 0;

  var greatestCommonDivisor = function(a, b) {
    if (b === 0) {
        return a;
    }

    return greatestCommonDivisor(b, (a % b));
  };

  var gcd = greatestCommonDivisor(finalDecimal, finalPower);

  // Since simplifyFraction() is now what we use for namespacing, it's not
  // kept as a function in here.
  if (Math.abs(i) > 0 && finalDecimal >= finalPower) {
    if (i < 0) {
      finalDecimal *= -1;    
    }

    var temp = improperFractionToMixedNumber(finalDecimal, finalPower);

    return [temp[0] + i, temp[1] / gcd, temp[2] / gcd];
  }

  return [i, finalDecimal / gcd, finalPower / gcd];
};

// The function no longer forces a certain outout. We get back the answer and treat it anyway we want.
//
var answer = simplifyFraction(21.8335745857, 2);

Answer 2

In addition to @Letharion's response, I would suggest copying the function Ratio.getRepeatProps() to get the value for decimalBeforeRepeatLength.

Ratio.getRepeatProps() - https://github.com/LarryBattle/Ratio.js/blob/master/lib/Ratio-0.4.0.js#L311

Example:

// Note that the repeating decimal must have the pattern occur at least twice for this to work.
// The pattern in this case is `35745857`
var parts = Ratio.getRepeatProps("21.833574585735745857");
// parts == ["21", "83", "35745857"]
// index 1 contains the numbers before the repeating pattern.
var decimalBeforeRepeatLength = (parts.length) ? parts[1].length : 0;