trjhtr

This program is a solution to homework given by our professor. So I want my program to be the most beautiful program ever written. Therefore I want to know where I could improve the quality of the code.

We have to write a class that represents a mathematical fraction. The class should also be able to add, substract, multiply and divide fractions. The fractions only have to work for positive integers.

Fraction.java

/* this class represents a mathematical fraction
 * only works for positive integers
 */

public class Fraction 
 private int numerator;
 private int denominator;
 // can be disabled to improve performance
 private boolean reduceFractionAutomatically;

 /* Constructors */

 public Fraction(int numerator, int denominator, boolean 
 reduceFractionAutomatically) 
 setNumerator(numerator);
 setDenominator(denominator);
 this.reduceFractionAutomatically = reduceFractionAutomatically;

 if(reduceFractionAutomatically) 
 reduceFraction();
 
 

 public Fraction(int numerator, int denominator) 
 this(numerator, denominator, true);
 

 /* Getters and Setters */

 private int getNumerator() 
 return numerator;
 

 private void setNumerator(int numerator) 
 if (numerator < 0) 
 numerator = 0;
 
 this.numerator = numerator;
 

 private int getDenominator() 
 return denominator;
 

 private void setDenominator(int denominator) 
 if (denominator <= 1) 
 denominator = 1;
 
 this.denominator = denominator;
 

 /* standard methods */

 @Override
 public String toString() 
 return String.valueOf(numerator) + "/" + String.valueOf(denominator);
 

 /* object-specific methods */

 public Fraction add(Fraction f) 
 int newDenominator = denominator * f.denominator;
 int newNumeratorOwnFraction = numerator * f.denominator;
 int newNumeratorElseFraction = f.numerator * denominator;
 return new Fraction(newNumeratorOwnFraction + newNumeratorElseFraction,
 newDenominator);
 

 public Fraction substract(Fraction f) 
 int newDenominator = denominator * f.denominator;
 int newNumeratorOwnFraction = numerator * f.denominator;
 int newNumeratorElseFraction = f.numerator * denominator;
 return new Fraction(newNumeratorOwnFraction - newNumeratorElseFraction,
 newDenominator); 
 

 public Fraction multiplicate(Fraction f) 
 int newDenominator = denominator * f.denominator;
 int newNumerator = numerator * f.numerator;
 return new Fraction(newNumerator, newDenominator);
 

 public Fraction divide(Fraction f) 
 Fraction reciprocal = new Fraction(f.denominator, f.numerator);
 return multiplicate(reciprocal);
 

 public void reduceFraction() 
 int divider = euklid(numerator, denominator);
 numerator /= divider;
 denominator /= divider;
 

 public int euklid(int a, int b) 
 if (a == 0) 
 return b;
 else 
 while (b != 0) 
 if (a > b) 
 a = a - b;
 else 
 b = b - a;
 
 
 
 return a;
 

Main.java

/* this class tests the functionalities of the fracture class */

public class Main 
 public static void main(String args) 
 Fraction f1 = new Fraction(45, 21);
 Fraction f2 = new Fraction(9, 12);

 // add
 Fraction f3 = f1.add(f2);
 System.out.println(f1 + " + " + f2 + " = " + f3);

 // substract
 f3 = f1.substract(f2);
 System.out.println(f1 + " - " + f2 + " = " + f3);

 // multiplicate
 f3 = f1.multiplicate(f2);
 System.out.println(f1 + " * " + f2 + " = " + f3);

 // divide
 f3 = f1.divide(f2);
 System.out.println(f1 + " / " + f2 + " = " + f3);
 

Output of the program

15/7 + 3/4 = 81/28
15/7 - 3/4 = 39/28
15/7 * 3/4 = 45/28
15/7 / 3/4 = 20/7

1. So, you're very close to an immutable Fraction class, which is great. Go the rest of the way! Make the reduce method return a new Fraction instance and you can make the class immutable. Then you get to explain to them the tradeoffs of immutability as a bonus - hopefully somebody will ask.




2. final is your friend. Well, it should be. It's great when I don't have to guess when a variable changes, because I know it won't.




3. Don't let people create an invalid Fraction, and don't change their input from something wrong to something else that's wrong. If they pass in an invalid value, throw a RuntimeException so they can fix the problem, rather than leaving them scratching their heads that negative fractions keep getting changed on them.




4. Don't label sections of code using comments. It's either unnecessary, because the code is reasonably-sized (this case), or it means the code is too complex and should be refactored.




5. Students need to learn to write documentation. That's Really Important. Nobody teaches it. You have no documentation. Even if they don't have to include it themselves, your "solution" should show them what it looks like.




6. As @vnp mentioned, name methods for what they do, not how they do it. The fact that the method use Euclid's GCD algorithm is an implementation detail. Also, I agree that the mod operator % makes it easier to read, and you're not going to overflow the stack computing a GCD. As a bonus, now you're not reassigning the method's parameters, which is a bad practice.




7. Your methods will read better to clients as plus, minus, times, and dividedBy.




8. If you're willing to make non-reduction the default instead of reduction, you can get rid of the reduce constructor all together and just say final Fraction f = new Fraction(1, 2).reduce(); I'd strongly recommend it, but I don't know what the requirements you gave your students were.




9. "The fractions only have to work for positive integers" - so you're promising not to test new Fraction(1, 2).minus(new Fraction(3,4));?

If you were to implement all of these changes, your code might look something like:

/**
 * Instances represent mathematical fractions. Only positive fractions are supported at this time. All instances
 * are immutable.
 */
public final class Fraction 

 private final int numerator;
 private final int denominator;

 /**
 * Constructor that allows you to specify whether or not to reduce the fraction.
 * @param numerator the numerator for the fraction. Must be >= 0.
 * @param denominator the denominator for the fraction. Must be > 0.
 * @param reduce true to reduce the fraction, false to leave it as is.
 * @throws IllegalArgumentException if either the numerator or denominator is invalid.
 */
 public Fraction(final int numerator, final int denominator, final boolean reduce) 
 if (numerator < 0) 
 throw new IllegalArgumentException("Numerator must be non-negative, was " + numerator);
 

 if (denominator <= 0) 
 throw new IllegalArgumentException("Numerator must be positive, was " + numerator);
 

 if (reduce) 
 final int gcd = this.greatestCommonDivisorOf(numerator, denominator);
 this.numerator = numerator / gcd;
 this.denominator = denominator / gcd;
 else 
 this.numerator = numerator;
 this.denominator = denominator;
 
 

 /**
 * Constructor. This fraction will be automatically reduced.
 * @param numerator the numerator for the fraction. Must be >= 0.
 * @param denominator the denominator for the fraction. Must be > 0.
 * @throws IllegalArgumentException if either the numerator or denominator is invalid.
 */
 public Fraction(final int numerator, final int denominator) 
 this(numerator, denominator, true);
 

 /**
 * @inheritDoc
 */
 @Override
 public String toString() 
 return String.valueOf(this.numerator) + "/" + String.valueOf(this.denominator);
 

 /**
 * Adds another fraction to this one. The resulting denominator will be the product of the two addends'
 * denominators.
 * @param addend the fraction to add to this one. May not be null.
 * @return a new Fraction instance that contains the result of the addition. Will never return null.
 */
 public Fraction plus(final Fraction addend) 
 return new Fraction(
 (this.numerator * addend.denominator) + (addend.numerator * this.denominator),
 this.denominator * addend.denominator);
 

 /**
 * Subtracts another fraction from this one. The resulting denominator will be the product of the denominators
 * of the minuend (this fraction) and subtrahend (parameter fraction).
 * @param subtrahend the fraction to subtract from this one. May not be null.
 * @return a new Fraction instance that contains the result of the addition. Will never return null.
 */
 public Fraction minus(final Fraction subtrahend) 
 return new Fraction(
 (this.numerator * subtrahend.denominator) - (subtrahend.numerator * this.denominator),
 this.denominator * subtrahend.denominator);
 

 /**
 * Multiplies this fraction by another fraction.
 * @param factor the fraction to multiply this one by. May not be null.
 * @return a new instance of a fraction containing the result of the multiplication. Will never return null.
 */
 public Fraction times(final Fraction factor) 
 return new Fraction(
 this.denominator * factor.denominator,
 this.numerator * factor.numerator);
 

 /**
 * Divides this fraction by another fraction.
 * @param divisor the fraction to divide this one by. May not be null.
 * @return a new instance of a fraction containing the result of the division. Will never return null.
 */
 public Fraction dividedBy(final Fraction divisor) 
 final Fraction reciprocal = new Fraction(divisor.denominator, divisor.numerator);
 return this.times(reciprocal);
 

 /**
 * Reduces this fraction to its simplest form by dividing the numerator and denominator by their greatest
 * common divisor.
 * @return a new instance of the fraction in reduced form. Will never return null.
 */
 public Fraction reduce() 
 final int gcd = this.greatestCommonDivisorOf(this.numerator, this.denominator);
 return new Fraction(this.numerator / gcd, this.denominator / gcd);
 

 private int greatestCommonDivisorOf(final int firstNumber, final int secondNumber) 
 if (secondNumber == 0) 
 return firstNumber;
 
 return this.greatestCommonDivisorOf(secondNumber, firstNumber % secondNumber);
 

• 
  

  Getters and setters are declared private, yet never called. Do you really need them?

  
  
  

  That said, I see no value they may have.

  
  



• euklid is normally called gcd. Note that the name shall reflect the purpose of the code, not the implementation details. Meanwhile, using a %
  operator seems more natural.



• 
  

  Names like newNumeratorElseFraction do not convey much meaning (at least for me), and just clutter the code. A straightforward

  
  
  

  public Fraction add(Fraction f) 
   return new Fraction(denominator * f.denominator + f.numerator * denominator,
   denominator * f.denominator);
  
  

  
  
  

  is much more readable.

  
  



• reduceFractionAutomatically doesn't make sense. Is an instance variable, yet every method calls the constructor in which it is defaulted to true. It would make certain sense as a static class variable (and you'd need to provide a way to reduce the fraction manually). However I don't think it worth the effort.