Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

After taking some great advice on this question, I'm hoping for some more feedback on my quadratic expression calculator.

This is my code as stands:

public class Quadratic_Equations {

    private final static Scanner sc = new Scanner(System.in);

    public static void main(String[] args) {
        while (true) {
            System.out.println("Please enter a, b and c or enter \"exit\" at any time to exit");
            double a = promptUserUntilValidInput("Please enter a:");
            double b = promptUserUntilValidInput("Please enter b:");
            double c = promptUserUntilValidInput("Please enter c:");
            System.out.println(formatOutput(calculateQuadraticFormula(a, b, c)));
            System.out.println();
        }
    }

    private static String formatOutput(Set<Double> resultsFromEquation) {
        if (resultsFromEquation == null)
            return "These numbers do not compute - they produce an illegal result.";
        else {
            return resultsFromEquation.toString();
        }
    }

    private static double promptUserUntilValidInput(String prompt){
        if (prompt.equals("exit")) {
            sc.close();
            System.exit(0);
        }
        boolean valid = false;
        Double value = new Double(0.0);
        while(!valid){
            System.out.println(prompt);
            value = tryParse(sc.nextLine());
            if (value != null)
                valid = true;
        }
        return value;
    }

    private static Double tryParse(String text){
        try{
            return Double.parseDouble(text);
        }
        catch (NumberFormatException e){
            return null;
        }
    }

    public static Set<Double> calculateQuadraticFormula(double a, double b, double c) {
        Set<Double> results = new HashSet<Double>();
        double temp = (b * b) - (4 * a * c);
        if (temp < 0)
            return null;
        if( temp == 0 ) {
            results.add(-b / 2 * a);} 
        else if( temp > 0) {
            results.add( (-b + Math.sqrt(temp) ) / (2 * a) );
            results.add( (-b - Math.sqrt(temp) ) / (2 * a) );
        }
        return results;
    }
}

Before I go on, there are a few things I should point out.

I'm aware this could be better if the quadratic stuff was in a class of its own - however, for the purposes of this simple exercise I'm happy skipping that.

My main issues are:

  1. static Scanner. Some people on previous question suggested making this a private variable, but then it needs to get passed to everything. Is this worth it?

  2. Does it look okay to close the scanner (and exit the program) where I do?

  3. Do other methods look single-responsibility-ish enough?

share|improve this question

2 Answers 2

The static Scanner is a problem, a better way to do it is with the Java try-with-resources. If you add the scanner as an input parameter to the prompt function, you can do:

Change your prompt method from:

private static double promptUserUntilValidInput(String prompt){

to

private static double promptUserUntilValidInput(Scanner sc, String prompt){

and then change your main-method to:

public static void main(String[] args) {
    try (Scanner sc = new Scanner(System.in)) {
        while (true) {
            System.out.println("Please enter a, b and c or enter \"exit\" at any time to exit");
            double a = promptUserUntilValidInput(sc, "Please enter a:");
            double b = promptUserUntilValidInput(sc, "Please enter b:");
            double c = promptUserUntilValidInput(sc, "Please enter c:");
            System.out.println(formatOutput(calculateQuadraticFormula(a, b, c)));
            System.out.println();
        }
    }
}

Instead of doing the System.exit, I would prefer a less abrupt exit. Consider a custom Exception, like "EndOfProgramException" which you can catch....

private static class EndOfProgramException extends RuntimeException {
    EndOfProgramException () {
        super();
    }
}

and throw this exception in your prompt method:

    if (prompt.equals("exit")) {
        throw new EndOfProgramException();
    }

and catch it in the main:

public static void main(String[] args) {
    try (Scanner sc = new Scanner(System.in)) {
        while (true) {
            System.out.println("Please enter a, b and c or enter \"exit\" at any time to exit");
            double a = promptUserUntilValidInput(sc, "Please enter a:");
            double b = promptUserUntilValidInput(sc, "Please enter b:");
            double c = promptUserUntilValidInput(sc, "Please enter c:");
            System.out.println(formatOutput(calculateQuadraticFormula(a, b, c)));
            System.out.println();
        }
    } catch (EndOfProgramException e) {
        System.out.prinln("Exiting...");
    }
}

As for the single-responsibility, it would look pretty clean with the above changes.

share|improve this answer
    
Sorry - how could I catch the exception in the prompt method? Currently I call the readLine method when calling tryParse. How would I fit the detect "exit" in there? –  user40096 Apr 4 at 21:19
    
Ahh... yes, I missed that. It is a bit of a hack, but adding it to the try-parse would be easy. Better would be to read the line before calling tryParse, and to check it then. –  rolfl Apr 4 at 21:23
    
I thought that - all advice so far has been to leave tryParse to do one thing. That's what I'll do –  user40096 Apr 4 at 21:25
    
Wherever you feel the urge to call System.exit(), throw an EndOfProgramExeption instead. That would cause the program flow to jump to the end of main(), where it would be caught. –  200_success Apr 6 at 12:06

Bugs

  • calculateQuadraticFormula() calculates wrong results for double roots. Watch your operator associativity!

    if( temp == 0 ) {
        results.add(-b / 2 * a);}
    
  • The "exit" feature doesn't work. The prompt would never be "exit", would it?

    private static double promptUserUntilValidInput(String prompt){
        if (prompt.equals("exit")) {
            sc.close();
            System.exit(0);
        }
    

Design issues

  • Class names should be nouns, and the name should reflect their purpose. I recommend QuadraticSolver as the class name.
  • The calculateQuadraticFormula() function could then be simply named solve().
  • The class should not hold a Scanner as a member variable, as that would violate the Single Responsibility Principle. It can be a local variable in main() that gets passed to the prompting function. (The scanner should be created using a try-with-resources block.)
  • Returning null from the solver is annoying for the caller to handle. I suggest returning an empty set if there are no real roots.
  • I suggest returning two copies of the result if it is a double root, but that is a matter for debate.

Style issues

  • You assign the expression (b * b) - (4 * a * c) to a variable named temp. Why not use a descriptive name discriminant?
  • The promptUserUntilValidInput() function could be better if you removed its tryParse() helper, I think. (Furthermore, promptUser… is redundant — who else are you going to prompt?)

    Also, since there is a System.exit() hidden inside, it needs to be documented. (Actually, @rolfl's suggestion to throw an exception is better.)

    /**
     * Prompts the user to enter a number, and retries until the input is a
     * valid double.  Calls System.exit(0) if the input is "exit", or if EOF
     * is encountered.
     */
    private static double promptUntilValidInput(String prompt, Scanner sc) {
        do {
            try {
                System.out.print(prompt);
                String input = sc.nextLine();
                if ("exit".equals(input)) {
                    System.exit(0);
                }
                return Double.parseDouble(input);
            } catch (NoSuchElementException eof) {
                System.out.println();
                System.exit(0);
            } catch (NumberFormatException retryOnBadInput) {
            }
        } while (true);
    }
    
share|improve this answer
    
Thank you for this. Can I ask what you mean when you say that returning the null from the solver is annoying for the caller to handle. After your suggestion I'm returning an empty set and then in the formatResult method I check if its empty. Why is this better? –  user40096 Apr 5 at 9:57
1  
Returning null forces the caller to check for a null. Returning an empty set lets the caller write simpler code, such as System.out.println("Equation has " + solve(a, b, c).size() + " real solutions."); That would be uglier if you returned null. Another way to think of it is that anything that removes the possibility of a NullPointerException is beneficial. –  200_success Apr 5 at 10:08

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.