Adding and subtracting vulgar fractions

Question

I'm trying to add two vulgar fractions together by finding the lowest common denominator and then adding. However, my code isn't behaving as expected, and is outputting two very high negative numbers. When I change the second fraction to 3/15 it outputs 0/0.

Here is my main program code:

class Program
{
    static void Main(string[] args)
    {
        Fraction n = new Fraction(2, 4);
        Fraction z = new Fraction(3, 12);

        Fraction sum = n.Add(z, n);
        int num = sum.Numerator;
        int den = sum.Denominator;

        Console.WriteLine("{0}/{1}", num, den);
        Console.ReadKey(true);
    }
}

Here is my Fraction class code:

internal class Fraction
{
    public Fraction(int numerator, int denominator)
    {
        Numerator = numerator;
        Denominator = denominator;
    }

    public int Numerator { get; private set; }

    public int Denominator { get; private set; }

    public Fraction Add(Fraction fraction2, Fraction fraction8)
    {
        int lcd = GetLCD(fraction8, fraction2);

        int x = lcd/fraction8.Denominator;
        int n = lcd/fraction2.Denominator;
        int f2num = fraction2.Numerator*n;
        int f8num = fraction8.Numerator*x;

        int t = fraction2.Numerator;

        Fraction Fraction3 = new Fraction(f2num+f8num,lcd);

        return Fraction3;
    }



    public int GetLCD(Fraction b, Fraction c)
    {
        int i = b.Denominator;
        int j = c.Denominator;

        while (true)
        {

            if (i == j)
            {
                return i;
            }
            j = j + j;
            i = i + i;
        }
    }
}

Good thing OP included all the `using` statements here. In all seriousness, you need to narrow down the issue a little bit here. It doesn't appear that you've put forth any effort whatsoever to debug this at all. Also, your variable names are horrendous. — tnw, Sep 25 '13 at 18:31
As above, GetLCD returns -2147483648 on my computer. Also, I don't think a while look is best here. — OMGtechy, Sep 25 '13 at 18:36
@DaveZych thanks for responding to my question. Can you elaborate a little more pls? — user2723261, Sep 25 '13 at 19:14
@user2723261 Look at Krishna's answer for a correct implementation. As a quick example, if you pass in 2 and 4 into your `GetLCD` method, first iteration checks `2 == 4`, second checks `4 == 8`, third checks `8 == 16` etc etc etc. They're never going to equal. I'm assuming the only reason something was returned was due to an overflow or something like that. — Dave Zych, Sep 25 '13 at 20:10

Venkata Krishna · Answer 1 · 2013-09-25T18:58:11.167

It didn't make sense to have GetLCD, Add & Subtract methods in the class. So, I moved it out of the class and made them static methods.

Your GetLCD function doesn't get LCD correctly. This will give you the required result.(I didn't bother to make the Subtract method working, you can follow the below code & make it work yourself)

PS: I didn't change all of your variable names & I would recommend you to make them as meaningful as possible. n,z,x,y,b,c are not good variable names

    static void Main(string[] args)
    {
        Fraction n = new Fraction(2, 4);
        Fraction z = new Fraction(3, 12);

        Fraction sum = Add(z, n);
        int x = sum.Numerator;
        int y = sum.Denominator;

        Console.WriteLine("{0}/{1}", x, y);
        Console.ReadKey(true);
    }

    public static Fraction Add(Fraction fraction2, Fraction fraction8)
    {

        int lcd = GetLCD(fraction8, fraction2);
        int multiplier = 0;
        if (fraction2.Denominator < lcd)
        {
            multiplier = lcd / fraction2.Denominator;
            fraction2.Numerator = multiplier * (fraction2.Numerator);
            fraction2.Denominator = multiplier * (fraction2.Denominator);
        }
        else
        {
            multiplier = lcd / fraction8.Denominator;
            fraction8.Numerator = multiplier * (fraction8.Numerator);
            fraction8.Denominator = multiplier * (fraction8.Denominator);
        }

        Fraction Fraction3 = new Fraction(fraction2.Numerator + fraction8.Numerator, lcd);

        return Fraction3;
    }

    public static int GetLCD(Fraction b, Fraction c)
    {
        int i = b.Denominator;
        int j = c.Denominator;

        int greater = 0;
        int lesser = 0;

        if (i > j)
        {
            greater = i; lesser = j;
        }
        else if (i < j)
        {
            greater = j; lesser = i;
        }
        else
        {
            return i;
        }
        for (int iterator = 1; iterator <= lesser; iterator++)
        {
            if ((greater * iterator) % lesser == 0)
            {
                return iterator * greater;
            }
        }
        return 0;
    }

    internal class Fraction
    {
        public Fraction(int numerator, int denominator)
        {
            Numerator = numerator;
            Denominator = denominator;
        }
        public int Numerator { get; set; }
        public int Denominator { get; set; }
    }

score 1 · Answer 2 · edited May 23 '17 at 12:11

Personally, I think your first mistake is trying to calculate the Lowest Common Denominator instead of just finding the simplest common denominator. Finding the LCD is a great stratgety for humans working this out on paper because of pattern recognition: we can recognize LCDs quickly; but calculating the LCD, and then converting the fractions to it is significantly more steps for a computer that must perform every step every time (and is not able to recognize patterns). Plus, once you add the two fractions after transforming them to their LCD it isn't even guaranteed to be a reduced result. I'm assuming that the reduced result is required, as is usually expected with fractional arithmetic. And because it seems useful, I put the reduction directly into the constructor code:

internal class Fraction
{
public Fraction(int numerator, int denominator, bool reduce = false)
{
    if (!reduce)
    {
        Numerator = numerator;
        Denominator = denominator;
    }
    else
    {
        var GCD = GreatestCommonDivisor(numerator, denominator);
        Numerator = numerator / GCD;
        Denominator = denominator / GCD;
    }
}

public int Numerator { get; private set; }

public int Denominator { get; private set; }    

public static Fraction Add(Fraction first, Fraction second)
{
    return Combine(first, second, false);
}

public static Fraction Subtract(Fraction first, Fraction second)
{
    return Combine(first, second, true);
}

private static Fraction Combine(Fraction first, Fraction second, bool isSubtract)
{
    var newDenominator = first.Denominator * second.Denominator;
    var newFirst = first.Numerator * second.Denominator;
    var newSecond = first.Denominator * second.Denominator;

    if (isSubtract)
    {
        newSecond = newSecond * -1;
    }

    return new Fraction(newFirst + newSecond, newDenominator, true);
}

private static int GreatestCommonDivisor(int a, int b) 
{
    return b == 0 ? a : GreatestCommonDivisor(b, a % b);
}

}

Edit: stole Greatest Common Divisor code from this answer

Adding and subtracting vulgar fractions

2 Answers2