5
\$\begingroup\$

I will begin this question with the disclaimer that it is homework but my code is written and working, I just want to know if there any ways in which I can improve what I already have. I want my programs to be as clean and easy to understand as possible.

My task is to Build two classes (Fraction and FractionCounter) and a Driver for use in counting the number of unique fractions read from a text file. Would it be more efficient to use ArrayList? If so, should it be an array to store the unique instances of fractions or should it store fractions that have no double instances? Are there other alternatives to this?

public class FractionDriver {
    public static void main(String[] args){

        // creates scanner object
        Scanner fractions = null;
        boolean [] repeated = new boolean [100]; //creates boolean array for repeats
        int repeat = 1; //sets repeat counter to 1

        // uses scanner to import fractions file and read how many lines 
        try{
            fractions = new Scanner(new FileInputStream("fractions.txt"));
        }catch(FileNotFoundException e){
            System.out.println("FILE NOT FOUND OR NOT OPENED");
            System.exit(0);
        }

        // creates a large array that stores data from the text file
        String[] input = new String[100];

        int n= 0; // counts the number of fractions
        int numElement = 0; // counts the current index element

        //loops through file to store data to input array
        while(fractions.hasNext()){
            input[numElement] = fractions.next();
            numElement++; //increments with each loop
            n++; //increments number of fractions
        }

        if (n == 0) { //if n = 0, nothing was found in the .txt file
            System.out.println("No fractions found.");
        }

        fractions.close(); // closes the input stream

        // create object list of fractions
        ObjectList fractionList = new ObjectList(n);
        ObjectList fracCount = new ObjectList(n);

        int totalFractions = 0;

        for(int i = 0; i < n; i++){ //loops through fractions

            // splits fractions to be stored in respective numerator and denom arrays
            String[] fract = input[i].split("/");

            // converts the string values to integers
            int numerator = Integer.parseInt(fract[0]);
            int denom = Integer.parseInt(fract[1]);

            // creates a fraction object and assigns instance variables
            Fraction f = new Fraction(numerator, denom);
            FractionCounter count = new FractionCounter(f);

            // adds the fraction to the array if the denominatoris not zero

            if(f.getDenominator() != 0){
                fractionList.add(f);

                for(int j = 0; j <=i; j++){ //loops through fractions

                    if(fracCount.get(j) != null){
                        if(!f.equals(fracCount.get(j))){ //checks if two values are equal
                            fracCount.add(f); //calls add method
                        }else{
                            count.compareAndIncrement(f); //calls compareAndIncrement() method
                            repeat++;//increments repeat value
                        }
                    }
                }   

                if (repeated[i] == false) { //checks if I has been printed
                    if(repeat == count.counted()) {
                        System.out.println(count.toString(f)); //calls toString() to print counts
                         repeated[i] = true;//says i has been printed

                    }
                }

            }
        }
    }
}

Here are the linked constructor classes.

Fraction counter:

public class FractionCounter extends Fraction{
    private Fraction theFraction;
    private int counter = 1; // sets starting value for counting variable

    FractionCounter(Fraction theFraction) {
        this.theFraction = theFraction;
    }

    /*
     * compares and increments values
     */
    boolean compareAndIncrement (Fraction newFraction) {
        if((theFraction.getNumerator() / theFraction.getDenominator() == 
            newFraction.getNumerator() / newFraction.getDenominator())){
            counter++; //increments
            return true;
        }else if(theFraction.getDenominator() == 0 || newFraction.getDenominator() == 0){
            return false;
        } else {
            return false;
        }
    }

    /*
     * checks if have been countes
     */
    public int counted () {
        return counter;
    }

    /*
     * returns as a string
     */
    public String toString(Fraction f) {
        return theFraction + " has a count of " + counter;
    }
}

Fraction Class:

public class Fraction {
    private int numerator; //only data elements
    private int denominator;

    /*
     * empty constructor
     */
    Fraction() {
    }

    /*
     * Sets numerator and denominator values
     */
    Fraction(int x, int y) {
        reduce(x,y);
    }

    /*
     * re duces numerator and denominator and checks if denominator euals zero,
     * in which case an error will be printed.
     */
    private void reduce(int numerator, int denominator) {
        int max = numerator; //sets max for GCD check
        int gcd = 1;

        //checks if both negative so it can be changed to positive
        if (numerator < 0 && denominator < 0) {
            this.numerator = Math.abs(numerator);
            this.denominator = Math.abs(denominator);
        }

        //checks for a divide by zero 
        if (denominator == 0) {
            System.err.println("Invalid fraction - cannot devide by zero"); //dividebyzeroexception????
            System.exit(0);
        }

        //finds GCD
        if (denominator > numerator) {
            max = denominator; 
        }

        for (int i = 1; i <= max; i++) {
            if (numerator % i == 0 && denominator % i == 0) {
                gcd = i;
            }
        } 

        //sets new values based on GCD
        if (gcd != 1) {
            this.numerator = numerator / gcd;
            this.denominator = denominator / gcd;
        } else {
            this.numerator = numerator;
            this.denominator = denominator;
        }

    }

    /*
     * checks for equality between passed Fraction object and current one
     */
    public boolean equals(Fraction other) {
        if (other == null) {
            return false;
        }
        if (!(other instanceof Fraction)) {
            return false;
        }
        Fraction that = other;
        return this.numerator == that.numerator && this.denominator == that.denominator;
    }

    /*
     * returns numerator
     */
    public int getNumerator() {
        return numerator;
    }

    /*
     * sets numerator to given value
     */
    public void setNumerator(int x) {
        numerator = x;
    }

    /*
     * returns denominator
     */
    public int getDenominator() {

        return denominator;
    }

    /*
     * sets denominator to given value
     */
    public void setDenominator(int y) {
        denominator = y;
    }

    /*
     * returns fraction as string
     */
    public String toString() {
        return numerator + "/" + denominator;
    }

}
\$\endgroup\$
5
\$\begingroup\$
  1. Use exceptions to handle exceptional situations. Don't just print something to the stderr and exit. For instance, if the denominator of a fraction is zero, it's reasonable to throw an IllegalArgumentException. The constructor should guarantee that a fraction is valid after the construction, but it shouldn't terminate the entire program if it's not either.

  2. Instead of parsing the input string in the main method, you can create a Fraction constructor that takes a string. It makes sense because a/b is a widely-accepted fraction representation.

  3. You code is not correct. For instance, it treats -1/2 and 1/-2 as different fractions. It means that you equals method is broken. You can fix it by changing the sign of the numerator and denominator if and only if the latter is negative.

  4. Do you need getters and setters in the Fraction class? Setters make no sense to me. You can break a fraction by setting denominator to 0. It breaks the integrity of this class. It's a good practice to keep your objects immutable. Just get rid of setters, you don't use them anyway.

  5. A default constructor for a Fraction is broken. It leaves the object in an invalid state (with 0 denominator). Either fix it you get rid of it.

  6. Why do pass numerator and denominator to the reduce function? Setting numerator and denominator fields in the constructor and working with them makes more sense.

  7. This: (theFraction.getNumerator() / theFraction.getDenominator() == newFraction.getNumerator() / newFraction.getDenominator()) is also broken. It's integer division. This way, 3/2 == 2/2. Don't use floating-point division, though. You've got the equals method in the Fraction class. Use it.

  8. Taking into account so many simple bugs, I have a question: did you test your code? Are you sure that it actually works? In any case, I'd recommend unit testing your code. Even simple small test cases are likely to catch many of the bugs you've got.

  9. If you want to count the number of occurrences of each fraction, you can use a Map instead of two nested loops. It'll make your code more efficient and readable. You'll need to override the hashCode method to use a HashMap to make it consistent with equals.

  10. What is the point of this code?

        else if(theFraction.getDenominator() == 0 || newFraction.getDenominator() == 0){
            return false;
        } else {
            return false;
        }
    

    It can be just

        else {
            return false;
        }
    
  11. You have a lot of variables in the main method. Some of them repeat one another: n and numElements are always equal. Get rid of one of them. I have no clue what repeated and repeat stand for. I have no idea why the size of the former is 100. What does that even mean? Frankly speaking, the main method is such a mess that I'd recommend deleting and completely rewriting it.

  12. The doc comments should be more detailed. Something like compares and increments values is meaningless. Compares what? Increments what? They are useless the way they're now.

  13. I suggest fully redesigning your solution:

    • The Fraction class should guarantee that the fraction is always in a valid state (if it's not, throw an exception). It should hide all implementation details from the user. It should be a solid entity, like a object of any standard java class. Implement a proper equals and hashCode methods. Remove all getters and setters (or at least setters). Unit test and document it properly.

    • The FractionCounter class can be much more simple: it can just take a list of fractions and count the number of occurrences using a hash map. There's no need to run two nested loops and compare the fractions in a weird way. Test and document it properly, too.

    • The Driver is now very simple, too: the main method can just reads the input, populate the list of fractions, passes its an instance of the FractionCounter and then prints the result. To make it even more manageable, you can factor reading and printing into separate methods. That's it. There's no need for strange boolean arrays with a fixed number of elements and tons of counters.

    • Name everything properly. For instance, fractions is a bad name for a Scanner. It sounds more like a list of fractions to me. You could call it scanner or inputScanner.

    • Use standard data structures that fit the intended purpose. Why is a list of fractions an instance of the ObjectList? It's not type safe. You can put something else in there. Use List<Fraction> instead (ArrayList<Fraction> is reasonable concrete implementation choice here).

\$\endgroup\$
  • \$\begingroup\$ this is all very helpful information and thank you for being so thorough in everything that I need to improve. \$\endgroup\$ – M Nico Jun 27 '17 at 17:59

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.