6
\$\begingroup\$

I've been making a program for an assignment that converts fractions to mixed numbers, and I'm done, but I was wondering if I could simplify it in any way, since it seems quite complicated right now.

Also, keep in mind that I am not allowed to use any Math or String methods to do this assignment, and I can only use +, -, *, and / as arithmetic operators. I also cannot use % or a modulus function.

I realize this is an assignment, and it's fine if you choose not to give me an answer. In fact, I'd actually prefer to have some tips or hints to help figure out how to solve it, rather than just telling me the answer.

Also, the 'c.print' is basically a print statement. I'm importing/using a different custom class, which is why it is different from the normal print statement.

    int numerator = 0; //can change to any value
int denominator = -2; //can change to any value
public int[] reducedFraction (int numerator, int denominator)
{
    int GCD = numerator;
    int tempN = numerator;
    int tempD = denominator;
    int[] fractionParts = {0, numerator, denominator};

    if (numerator == 0) //if the numerator is 0
    {
        return fractionParts;
    }

    //Making sure the program works with negative values by temporarily converting GCD and fractionParts[2] to positive values
    GCD = (numerator < 0) ? -numerator:
    numerator;

    tempD = (denominator < 0) ? -denominator:
    denominator;

    //Finding the GCD of the numerator and denominator
    while (GCD != tempD)
    {
        if (GCD > tempD)
            GCD -= tempD;
        else
            tempD -= GCD;
    }

    //Simplfying numerator and denominator
    fractionParts [1] /= GCD;
    fractionParts [2] /= GCD;

    //Temporarily making the numerator and denominator positive
    tempN = (fractionParts [1] < 0) ? -fractionParts [1]:
    fractionParts [1];

    tempD = (fractionParts [2] < 0) ? -fractionParts [2]:
    fractionParts [2];

    //if the numerator is not greater than the denominator return the simplified fraction.
    if (tempN < tempD)
    {
        return fractionParts;
    }

    //Finding value of whole number, numerator and possibly converting denominator to original sign
    fractionParts [0] = fractionParts [1] / fractionParts [2];
    fractionParts [1] = fractionParts [1] - (fractionParts [1] / fractionParts [2]) * fractionParts [2];

    if (fractionParts [1] < 0 && fractionParts [2] < 0) //if the numerator and denominator are both negative
    {
        fractionParts [1] = (fractionParts [1] < 0) ? -fractionParts [1]:
        fractionParts [1];

        fractionParts [2] = (fractionParts [2] < 0) ? -fractionParts [2]:
        fractionParts [2];
    }

    return fractionParts;
}


public void display ()
{
    if (reducedFraction (numerator, denominator) [1] == 0)
        c.print (reducedFraction (numerator, denominator) [0]);

    else if (reducedFraction (numerator, denominator) [0] == 0)
        c.print (reducedFraction (numerator, denominator) [1] + "/" + reducedFraction (numerator, denominator) [2]);

    else
        c.print (reducedFraction (numerator, denominator) [0] + " " + reducedFraction (numerator, denominator) [1] + "/" + reducedFraction (numerator, denominator) [2]);
}
\$\endgroup\$
4
\$\begingroup\$

First, a couple of tips:

  • Using an int[] to represent the result is a poor choice. It would be better to create a class for this, for example called MixedNumber. Then, instead of accessing the elements by indexes, you can access them by field name. It could also have an appropriate toString method to print itself

  • The handling of negative numbers is very poor. You could determine the sign of the number in one place early on, use it to multiply the integral part and that's it. That would get rid of many confusing redundant logic. The current code also has a bug related to it: for the input (3, -2) it gives back -1 1/-2 instead of -1 1/2

  • An elegant implementation of the gcd algorithm is this recursive function: return b == 0 ? a : gcd(b, a % b);

  • The printing of the number can be cleaner and simpler using String.format

I recommend to rewrite the function in terms of this class:

class MixedNumber {
    private final int integral;
    private final int numerator;
    private final int denominator;

    private MixedNumber(int integral, int numerator, int denominator) {
        this.integral = integral;
        this.numerator = numerator;
        this.denominator = denominator;
    }

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

    static MixedNumber fromFraction(int numerator, int denominator) {
        // TODO: simplified main implementation comes here
    }

    @Override
    public String toString() {
        if (numerator == 0) {
            return "" + integral;
        } else if (integral == 0) {
            return String.format("%d/%d", numerator, denominator);
        } else {
            return String.format("%d %d/%d", integral, numerator, denominator);
        }
    }
}

Rather than printing the output, use unit tests to verify. After you finished the implementation above, with the bug that I mentioned fixed, these unit tests should pass:

@Test
public void testZeroNumerator() {
    assertEquals("0", MixedNumber.fromFraction(0, -2).toString());
}

@Test
public void test_minus3_over_2() {
    assertEquals("-1 1/2", MixedNumber.fromFraction(-3, 2).toString());
}

@Test
public void test_3_over_minus2() {
    assertEquals("-1 1/2", MixedNumber.fromFraction(3, -2).toString());
}

@Test
public void test_minus3_over_minus2() {
    assertEquals("1 1/2", MixedNumber.fromFraction(-3, -2).toString());
}

@Test
public void test_123_over_82() {
    assertEquals("1 1/2", MixedNumber.fromFraction(123, 82).toString());
}

@Test
public void test_82_over_123() {
    assertEquals("2/3", MixedNumber.fromFraction(82, 123).toString());
}

Whatever IDE you use, there is certainly an easy way to add a JUnit4 test class and run it. Give it a try!

\$\endgroup\$
3
\$\begingroup\$

Here are a few suggestions for changes:

(1) Instead of array name with 3 elements use 3 variable names (whole, numerator, denominator). fractionParts array name is less descriptive.

(2) Use modulo operator %. Based on (1) and (2), code segment

//Finding value of whole number, numerator and possibly converting denominator to original sign
fractionParts [0] = fractionParts [1] / fractionParts [2];
fractionParts [1] = fractionParts [1] - (fractionParts [1] / fractionParts [2]) * fractionParts [2];

becomes shorter and easier to read. Example of how % operator fits in is in the comments.

whole = numerator / denominator;       // e.g. 0 10/3:  n=10 / d=3 => w=3
numerator %= denominator;              // e.g.          n=10 % d=3 => n=1     0 10/3 becomes 3 1/3

(3) Create and use gcd method. Main code will become a little shorter and it will be easier to use the GCD code elsewhere.

private int gcd(int a, int b) {
    if (b == 0) return a;
    return gcd(b, a%b);
}
\$\endgroup\$
  • 3
    \$\begingroup\$ w, n, d? @janos's suggestion of having the right return type is better: the issue is not with lengthy array names, but the choice of using an array to represent the result. \$\endgroup\$ – h.j.k. Nov 12 '14 at 7:22
  • \$\begingroup\$ Using custom type and JUnit are both excellent suggestions. Suggestion to use short and meaningful local variable names still applies.. \$\endgroup\$ – Andrej Nov 12 '14 at 8:09
  • 1
    \$\begingroup\$ IMO, one letter is too short, as the names aren't meaningful unless commented. When you look at them six months from now (which may not apply here but is normal in development), you'll have to think about why you chose them. Writing out whole, numerator, and denominator will save time in the long run. No thinking about why the names were chosen. No reference to comments. Just good self-commenting code that reads naturally. \$\endgroup\$ – Brythan Nov 12 '14 at 8:57
  • \$\begingroup\$ The idea of making a seperate method for the GCD is great, thanks for that! However, as stated in the question, I'm only allowed to use the basic arithmetic operators, which is why I didn't use % to begin with :( \$\endgroup\$ – atarw Nov 12 '14 at 12:47
  • 1
    \$\begingroup\$ @dabigone Because dividing ints discards the fractional part, a % b is equivalent to a - (a / b) * b if a and b are both ints. \$\endgroup\$ – cbojar Nov 13 '14 at 4:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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