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 JavaScript that takes 2 fractions and finds the common denominator for both. If the user inputs 2/6 and 1/2 --- the script outputs 2/6 + 3/6. I'm interested in feedback on possibly simplifying it or improving it. Thanks.

JS Bin: http://jsbin.com/omelen/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() {

    // Finds the highest common factor of 2 numbers
    function highestCommonFactor(a, b) {
        if (b == 0) {
            return a;
        }

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

    // Input fractions to add ////////////////////////////////
    // Fraction 1 = 2/6
    var fraction1Numerator = 2;
    var fraction1Denominator = 6;

    // Fraction 2 = 1/2
    var fraction2Numerator = 1;
    var fraction2Denominator = 2;
    ////////////////////////////////////////////////////////////////

    // Find the highest common factor of both denominators
    var factor = highestCommonFactor(fraction1Denominator, fraction2Denominator); // 2

    if (factor > 1) { // There's a common factor greater than 1
        if (fraction1Denominator > fraction2Denominator) {
            // Find out how many times the factor goes into bigger denominator
            var temp = fraction1Denominator / factor;

            fraction2Numerator  = fraction2Numerator * temp;
            fraction2Denominator = fraction2Denominator * temp;
        } else {
            // Find out how many times the factor goes into bigger denominator
            var temp = fraction2Denominator / factor;

            fraction1Numerator  = fraction1Numerator * temp;
            fraction1Denominator = fraction1Denominator * temp;
        }
    } else { // There's no common factor greater than 1 so we need to multiple each fraction by each others denominators
        // Temp values
        var fraction1NumeratorTemp = fraction1Numerator;
        var fraction1DenominatorTemp = fraction1Denominator;

        fraction1Numerator  = fraction1Numerator * fraction2Denominator;
        fraction1Denominator = fraction1Denominator * fraction2Denominator;

        fraction2Numerator  = fraction2Numerator * fraction1DenominatorTemp;
        fraction2Denominator = fraction2Denominator * fraction1DenominatorTemp;
    }

    // Display solution
    document.getElementById("divSolution").innerText = fraction1Numerator + "/" + fraction1Denominator + " + " + fraction2Numerator + "/" + fraction2Denominator;
}
</script>
</head>

<body>
<div id="divSolution"></div>
</body>
</html>
share|improve this question

1 Answer 1

up vote 2 down vote accepted

Here's my attempt to help you.

  • As a general rule, keep everything as general as you can and don't split your code into different cases unless you have to. From the if (factor >1) part, I knew things could be improved
  • At the end, the denominators will be the Least Common Multiple of the two initial fractions.
  • Then, you just need to multiply the numerators by the same amount used to multiply the denominators

Here's the corresponding code.

// Finds the highest common factor of 2 numbers
function highestCommonFactor(a, b) {
    if (b == 0) {
        return a;
    }
    return highestCommonFactor(b, a%b);
}
function leastCommonMultiple(a,b) {
    return a*b/(highestCommonFactor(a,b));
}

// Input fractions to add ////////////////////////////////
// Fraction 1 = 2/6
var fraction1Numerator = 2;
var fraction1Denominator = 6;

// Fraction 2 = 1/2
var fraction2Numerator = 1;
var fraction2Denominator = 2;
////////////////////////////////////////////////////////////////

// Find the highest common factor of both denominators
var commonMultiple = leastCommonMultiple(fraction1Denominator, fraction2Denominator); 

fraction1Numerator   *= (commonMultiple / fraction1Denominator);
fraction2Numerator   *= (commonMultiple / fraction2Denominator);

// Display solution
document.getElementById("divSolution").innerText = fraction1Numerator + "/" + commonMultiple + " + " + fraction2Numerator + "/" + commonMultiple;
share|improve this answer
    
Thanks for taking a look at my code. I tried your revised code but had some issues... I inputted the following fractions and got the following incorrect results...Fraction 1 = 1/2 --> 1/6 Fraction 2 = 2/6 --> 6/6; Fraction 1 = 2/12 --> 2/36 Fraction 2 = 1/18 --> 6/36; Fraction 1 = 1/18 --> 1/36 Fraction 2 = 2/12 --> 12/26. –  user1822824 Jan 9 '13 at 5:21
    
I changed the following 2 lines of your code: fraction2Numerator *= (commonMultiple / fraction1Denominator); fraction2Numerator *= (commonMultiple / fraction2Denominator); --- TO ---> numeratorMultipler1 = commonMultiple / fraction1Denominator; numeratorMultipler2 = commonMultiple / fraction2Denominator; fraction1Numerator = numeratorMultipler1 * fraction1Numerator; fraction2Numerator = numeratorMultipler2 * fraction2Numerator; And that fixed the issue. Let me know what you think? –  user1822824 Jan 9 '13 at 5:24
    
Actually, you just need to change : fraction2Numerator *= (commonMultiple / fraction1Denominator); for fraction1Numerator *= (commonMultiple / fraction1Denominator); (one character is to be changed) –  Josay Jan 9 '13 at 6:01
    
Also, I think before programming anything, you should try to do your calculation with a pen and paper and try to figure out what you are doing. Then, once everything's clear, you can open your favorite text editor. –  Josay Jan 9 '13 at 6:25
    
I see it now. Thanks. I started learning JavaScript a few months ago, I appreciate your feedback, especially on how I can make my code clearer and more efficient. I plan on posting a few more similar fraction/decimal codes for review and would like your feedback if you have time. –  user1822824 Jan 9 '13 at 7:20

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.