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.

I wrote a simple script that converts a decimal to a mixed number, proper, or improper fraction depending on the inputed decimal.

It works but I think it could be improved as it hangs when large decimals are used. Please review and let me know how I could improve and simplify it. Thanks.

My script on JS Bin: http://jsbin.com/axulob/1/edit

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />
<title>Untitled Document</title>
<script>
window.onload = function() {

    var factor; 

    // Finds the highest common factor of 2 numbers
    function highestCommonFactor() {
        for (factor = numerator; factor > 0; factor--) {
            if ((numerator % factor == 0) && (denominator % factor == 0)) {
                return factor;
            }
        }
    }

    // Enter a decimal to convert to a fraction
    var decimal = "1.75";

    // Split the decimal
    var decimalArray = decimal.split(".");

    var leftDecimalPart = decimalArray[0];
    var rightDecimalPart = decimalArray[1];

    // Save decimal part only for later use
    var decimalOnly = "0." + rightDecimalPart;

    // Find the decimal multiplier
    var multiplier = "1";

    for (var i = 0; i < rightDecimalPart.length; i++) {
        multiplier += "0";
    }

    // Create numerator by multiplying the multiplier and decimal part together
    var numerator = Number(multiplier) * Number(decimalOnly);

    var denominator = multiplier;

    // Find the highest common factor for the numerator and denominator
    highestCommonFactor();

    // Simplify the fraction by dividing the numerator and denominator by the factor
    var numerator = Number(numerator) / Number(factor);
    var denominator = Number(denominator) / Number(factor);

    // Output as a mixed number fraction (depending on input)
    var mixedNumber = leftDecimalPart + " " + numerator + "/" + denominator;

    // Output as a proper fraction or improper fraction (depending on input)
    var numerator = numerator + (leftDecimalPart * denominator);
    var fraction = numerator + "/" + denominator;

    // Display solution
    document.getElementById("divSolution").innerText = fraction;
}
</script>
</head>

<body>
<div id="divSolution"></div>
</body>
</html>
share|improve this question
    
Why not use an existing fraction library instead? Here's one github.com/LarryBattle/Ratio.js –  Larry Battle Jan 10 '13 at 16:38
add comment

1 Answer

up vote 2 down vote accepted
  1. highestCommonFactor should take 2 integers as parameter instead of relying on the variable numerator and `denominator'. Also, you could find it using Euclid'a algorithm.

  2. I am wrong is saying that this piece of code :

    var rightDecimalPart = decimalArray[1];
    // Save decimal part only for later use
    var decimalOnly = "0." + rightDecimalPart;
    // Find the decimal multiplier
    var multiplier = "1";
    for (var i = 0; i < rightDecimalPart.length; i++) {
        multiplier += "0";
    }
    // Create numerator by multiplying the multiplier and decimal part together
    var numerator = Number(multiplier) * Number(decimalOnly);
    

is here to transform a number such as 78924 in 0.78924 and then check that have to multiply it by 100000 to get an integer which is ... 78924.

Edit After a first cleanup, I get :

function highestCommonFactor(a,b) {
    if (b==0) return a;
    return highestCommonFactor(b,a%b);
}

var decimal = "1.75";
var decimalArray = decimal.split(".");
var leftDecimalPart = decimalArray[0];
var rightDecimalPart = decimalArray[1];

var denominator = "1";

for (var i = 0; i < rightDecimalPart.length; i++) {
    denominator += "0";
}
document.getElementById("debug").innerText = denominator;
var factor = highestCommonFactor(rightDecimalPart, denominator);

// Simplify the fraction by dividing the numerator and denominator by the factor
var denominator = Number(denominator) / Number(factor);
var numerator = (Number(rightDecimalPart) / Number(factor)) + (leftDecimalPart * denominator);

// Display solution as a proper fraction or improper fraction (depending on input)
document.getElementById("divSolution").innerText = numerator + "/" + denominator;

I'll try to go a step further.

Edit 2 After a rewriting of the calculation, here's what I got :

function highestCommonFactor(a,b) {
    if (b==0) return a;
    return highestCommonFactor(b,a%b);
}

var decimal = "1.75";
var decimalArray = decimal.split(".");
var leftDecimalPart = decimalArray[0]; // 1
var rightDecimalPart = decimalArray[1]; // 75

var numerator = leftDecimalPart + rightDecimalPart // 175
var denominator = Math.pow(10,rightDecimalPart.length); // 100
var factor = highestCommonFactor(numerator, denominator); // 25
denominator /= factor;
numerator /= factor;

document.getElementById("divSolution").innerText = numerator + "/" + denominator;
share|improve this answer
    
Thanks for taking the time to go through my code. Could you explain how the function highestCommonFactor works? –  user1822824 Jan 8 '13 at 21:31
    
I understand Euclid'a algorithm but I don't get how it's returning 2 values (return highestCommonFactor(b,a%b))? I assume it's multiplying (b) * ((a%b)). –  user1822824 Jan 8 '13 at 22:39
    
The method highestCommonFactor takes 2 (integer) arguments and return 1 (integer) value. I used the formula from en.wikipedia.org/wiki/… to write this recursive function. –  Josay Jan 9 '13 at 0:23
add comment

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.