Quadratic expression calculator

Question

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.

Answer 1

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;
     }
}

Answer 2

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.

Answer 3

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;
    }

Answer 4

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).

Answer 5

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.

Answer 6

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.

Answer 7

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.