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.

I'm hoping to get some feedback on how to improve and optimise a program I wrote which takes three values as input (via the terminal) and uses them in the quadratic formula.

I'm aware this is mostly micro-optimisation, but any advice whatsoever would be much appreciated.

public class Quadratic_Equations {
    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 -1 at any time to exit");
            double a = tryParse(sc.nextLine());
            a = checkIfValidNumber(a);
            if (a == -1)
                break;
            double b = tryParse(sc.nextLine());
            b = checkIfValidNumber(b);
            if (b == -1)
                break;
            double c = tryParse(sc.nextLine());
            c = checkIfValidNumber(c);
            if (c == -1)
                break;

            System.out.println(useQuadraticFormula(a, b, c));
            System.out.println();
        }
        sc.close();
    }

    private static double checkIfValidNumber(double number) {
        while (number == -1.0) {
            System.out.println("You didn't enter a valid number for a. Please try again.");
            number = tryParse(sc.nextLine());
        }
        return number;
    }

    public static Double tryParse(String text) {
        try {
            return new Double(text);
        } catch (NumberFormatException e) {
            return -1.0;
        }
    }

    public static String useQuadraticFormula(double a, double b, double c) {
        double result1;
        double result2;
        if (Math.pow(b, 2) - (4 * a * c) <= 0)
            return "These numbers do not compute - they produce an illegal result.";
        result1 = (-b + Math.sqrt(Math.pow(b, 2) - (4 * a * c))) / (2 * a);
        result2 = (-b - Math.sqrt(Math.pow(b, 2) - (4 * a * c))) / (2 * a);
        return String.valueOf(result1) + ", " + String.valueOf(result2);
    }
}

Among the things I really dislike are:

  1. The static scanner object.
  2. The three different checks for -1 being entered.
  3. The quadratic formula method, which seems like it could be shortened.

I'm still working on this myself, but as I said any advice (including on naming conventions, code style etc.) would be appreciated.

share|improve this question
    
There is also a follow-up question. –  200_success Apr 6 at 6:51
add comment

7 Answers 7

A few things jump me here:

1. Class Names

Your class name is queer. The Java standard is to use PascalCasing for class names. You use Pascal_Snake_Case. That's kinda funny ;) Also you named your class after what it contains somewhere deep down, and not what it's responsible for. I'd instead probably name it Program. As it's nothing else, than that. I'd expect a class named Quadratic_Equations to be an enum containing different Equations.

2. checkIfValidNumber

I like the name. It's good, it could describe what it does. I don't like the print-statement and that you have a loop in there. This is one of these "side-effects". Your function name makes me expect something different. I expect a function applying some constraint to a number and returning true or false depending on the validity of that number to the constraint.

You also hide your whole process behind that. Also: why should a user not enter -1.0 for your quadratic equation? It makes no sense to use a number within the valid range to check for validity. instead use boolean return type in your tryParse and make use of the out-parameter.

3. Reading user input.

double a = tryParse(sc.nextLine());
  a = checkValidNumber(a);

This makes no sense. You should also move the first tryParse(sc.nextLine()); into a method.

Instead I would expect:

double a = promptUserUntilValidInput("Please enter a:");

You can move the whole reading to this method.

private double promptUserUntilValidInput(String prompt){
    boolean valid = false;
    double value = 0.0;
    while(!valid){
       System.out.println(prompt);
       //this is the c# attempt...
       valid = tryParse(sc.nextLine(), out value);

       //You would need to do something like this in java
       Double result = tryParse(sc.nextLine());
       valid = result != null;
       value = (double)result;
    }
    return value;
}

tryParse would then (in C#) return true, when parsing the number was successful. You'd just need to assign to an out parameter before returning in the method:

private boolean tryParse(String text, out double val){
     try{
        val = new Double(text);
        return true;
     }
     catch (NumberFormatException e){
        return false;
     }
}

This way you also eliminate all checks for -1.0.

But as this is java we need to make it a little more complicated. I'm here heavily relying on @Uri Agassi's answer

private Double tryParse(String text){
    try{
       Double value = new Double(text);
       return value;
    }
    catch (NumberFormatException e){
       if(text.matches("/^quit$/gi"))
           Application.Exit;
       else
         return null;
     }
}
share|improve this answer
2  
this is java - no out parameter... –  Uri Agassi Apr 4 at 17:59
1  
lol, you can pass a container, but think how ugly it will look to use it... In my answer I suggested boxing, and returning null - dotnetbutchering.blogspot.co.il/2012/07/… –  Uri Agassi Apr 4 at 18:04
    
I would throw an exception and catch it. It's rather an exceptional case when the user enters an invalid input. –  palacsint Apr 4 at 19:23
    
Pascal_Snake_Case, LOL :-) –  palacsint Apr 4 at 19:24
1  
You should fix the last part, or at least remark that it doesn't work in java: codereview.stackexchange.com/questions/46293/… –  Uri Agassi Apr 5 at 5:16
show 1 more comment

When implementing even such a simple code I'd suggest thinking of how you'd like to test it in the automated way (i.e. using standard Java testing framework - JUnit). It will guide you to a better design and in consequence improve your code.

While there is plenty of improvements which can still be done to the attached code as a whole, I will focus on the useQuadraticFormula method as it hasn't been yet extensively commented upon:

  • Firstly the purpose of this method is unclear, as it returns a formatted string. It would be much better to have a method calculateQuadraticFormula which would return the collection of double values (ideally not array, but a dynamic size collection - java.util.Set would be good as it eliminates the duplicates, which can happen if b * b == 4 * a * c). This would allow to test the result of the method (to check whether you received 0, 1 or 2 results and whether the specific values are correct). Then you can format the results to user-friendly string in another function (which also can be tested).

  • As already advised, it makes sense to define the interim value:

    double someMeaningfulName = Math.sqrt(Math.pow(b, 2) - (4 * a * c))
    

    You can also use import static Math.* which will allow you to write the formula with less noise:

    double someMeaningfulName = sqrt(pow(b, 2) - (4 * a * c))
    
  • Finally, using pow(b, 2) is a bit over the top. Instead, b * b is as good, more readable and slightly faster.

Please find below the example making use of my comments:

import java.util.Set;
import java.util.HashSet;

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 ) {
            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;
    }
share|improve this answer
    
Could you provide any examples of how such a Set method would work for calculating the Quadratic Formula? –  user40096 Apr 4 at 19:49
    
I've edited my comment to attach the example. Please also note, that it makes sense to split the if >= 0 into 2 separate cases, firstly for performance reasons (avoid duplicate computation), but mostly due to rounding issues with floating point number representation. Therefore in some odd cases you might end up with 2 results (i.e. 9.0000000001 and 8.9999999998) when 1 is expected. You can avoid it by rounding the double results to the number of decimal places making sense for the user. –  Norbert Apr 4 at 20:21
add comment

Regarding the Scanner object:

You're right. I also don't like that. For two reasons.

  1. It's not private final. It's good to restrict things as much as possible (making things private). And as the object reference never changes, it's good practice to mark it as final.

    private static final Scanner sc = new Scanner (System.in);
    
  2. It can be a local variable :) It doesn't even need to be private static final. Create it in the main method and then pass it as a parameter to any method that needs it.


Shortening useQuadraticFormula

How many times are you using Math.pow(b, 2) - (4 * a * c)? THREE! It's about time to put it in a variable, don't you think?

You can also do the initialization and declaration of resul1 and result2 respectively on the same line.

public static String useQuadraticFormula(double a, double b, double c) {
    double temp = Math.pow(b, 2) - (4 * a * c);
    if (temp <= 0)
        return "These numbers do not compute - they produce an illegal result.";
    double result1 = (-b + Math.sqrt(temp)) / (2 * a);
    double result2 = (-b - Math.sqrt(temp)) / (2 * a);
    return String.valueOf(result1) + ", " + String.valueOf(result2);
}

Object oriented

Overall, a good continued development of this would be to make things more object oriented. Create your own class, QuadraticFormula that contains the values of a, b, c. Use a method named computeResult which could return either an integer array (int[]) of length two for the two results, or another custom class, QuadraticResult that could hold the two results, and possibly some of the temporary variables used in-between.

Exceptions

return "These numbers do not compute - they produce an illegal result.";

If it is illegal, then

throw new IllegalArgumentException("These numbers produce an illegal result.");

Do not return a "custom constant for 'things went wrong'" This is really one of those cases when using an exception makes sense.

share|improve this answer
    
Thank you for these suggestions. The reason why I didn't throw an exception like in your last line is because it stops the program running, whereas the return statement prints the string, then begins the program over again. –  user40096 Apr 4 at 19:35
    
@Andrew Then you can catch (IllegalArgumentException e) –  Simon André Forsberg Apr 4 at 19:50
    
Sorry - how can I do that? I'm assuming I need a try and catch block, but what would the try statement be? –  user40096 Apr 4 at 19:55
add comment
  1. When you need a double don't create a Double object.

    double a = tryParse(sc.nextLine()); //<-- tryParse returns Double, causes autounboxing
    

    Use double Double.parseDouble(String s) instead.

    public static double tryParse(String text) {
        try {
            return Double.parseDouble(text);
        } catch (NumberFormatException e) {
            return -1.0;
        }
    }
    
  2. Simplify String concatenation.

    When left argument of + is a String you don't need String.valueOf, at least you want apply some format.

    In useQuadraticFormula you could write

    return String.valueOf(result1) + ", " + result2;
    

    You could event write

    return "" + result1 + ", " + result2;
    

    When lot of String concatenation are involved, use String.format or StringBuilder (or StringBuffer if thread safety is required).

share|improve this answer
add comment

checkIfValidNumber() sounds like it is returning a boolean value if the number is valid. Maybe parseValidNumber() throws IllegalNumberException is better.

I would create the Scanner in main() and pass it on to every function that needs it, although it does not seem neccessary here.

Using a Constructor instead of doing all in main() would be good too.

share|improve this answer
    
checkNotNull from Guava's Preconditions also returns the input value instead of returning boolean. It makes the following possible: this.x = checkNotNull(x, "x cannot be null"); (Anyway, welcome on the site and +1 :) –  palacsint Apr 5 at 9:37
add comment

Who should use scanner?
As was already suggested - sc should be a local variable. I don't agree with the suggestion to pass it around as an argument, since I don't think the other methods have any business prompting the user. checkIfValidNumber() should do exactly that - check that the number is valid!

What's the meaning of -1?
From reading your code, I can see a little problem:

if (a == -1) // or b or c
    break;

This if will never be true - as -1 is considered an indicator that it is invalid, and checkIfValidNumber will then prompt the user for a new number...
It is unfortunate that java does not have TryParse like in C#... You can, however use boxing to indicate an invalid number as null. This way - all valid numbers are fine, including -1.

share|improve this answer
    
If I use scanner as local variable, I've two questions. 1: If I close it at end of method, is this okay (as it will basically instantiate and close scanner three times, for three different inputs). 2: If inside the tryParse I detect if the user enters -1 (to exit program) and do a System.exit, does it matter that the Scanner hasn't been closed? –  user40096 Apr 4 at 19:41
    
1. In my suggestion, I meant for the scanner to be a local variable of main(), not any other method, so you close it in main() - once. 2. You should not make any decision regarding -1 inside tryParse - simply return the number. It is the decision of the part which manages the application flow (main()) to check for the -1, and decide whether to exit. –  Uri Agassi Apr 4 at 20:00
    
See my updated code. I can change the scanner to be in the main, but I'll have to pass it as an argument then to the prompt user method. Additionally, if I check for "-1" (or "exit" as I now call it) inside the main method, I have to check three times (After each entry) as opposed to once in tryPArse method. Isn't there a better way? –  user40096 Apr 4 at 20:02
    
tryParse should do what it says - and nothing more. You can check for "exit" before you pass it to tryParse. Btw, your tryParse will not work, since arguments in java are passed by pointer, not by reference - value in promptUserUntilValidInput will always return 0.0 (the initial value) –  Uri Agassi Apr 4 at 20:10
    
Just realised that. I was following the suggestion from the top rated answer, but I don't think it works. –  user40096 Apr 4 at 20:15
add comment

In addition to the other comments, I suggest you check for a == 0.0 (comparing doubles against zero is about the only safe comparison you can do). If a == 0.0 there is a rather simpler way to solve the equation, and the normal quadratic solution will give a division by zero. You probably need to special case (a == 0.0) && (b == 0.0) too.

share|improve this answer
add comment

Your Answer

 
discard

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