2
\$\begingroup\$

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>
Improve this question
\$\endgroup\$

1 Answer 1

Reset to default
2
\$\begingroup\$

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;
Improve this answer
\$\endgroup\$
6
  • \$\begingroup\$ 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. \$\endgroup\$
    – user1822824
    Jan 9, 2013 at 5:21
  • \$\begingroup\$ 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? \$\endgroup\$
    – user1822824
    Jan 9, 2013 at 5:24
  • \$\begingroup\$ Actually, you just need to change : fraction2Numerator *= (commonMultiple / fraction1Denominator); for fraction1Numerator *= (commonMultiple / fraction1Denominator); (one character is to be changed) \$\endgroup\$
    – SylvainD
    Jan 9, 2013 at 6:01
  • \$\begingroup\$ 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. \$\endgroup\$
    – SylvainD
    Jan 9, 2013 at 6:25
  • \$\begingroup\$ 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. \$\endgroup\$
    – user1822824
    Jan 9, 2013 at 7:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.