Quadratic Calculator

Question

I am about a month into learning Java and I'm just looking for some comments on my code. Specifically, I would like to know if I'm being as efficient as I can be. This is the most complex thing I've come up with so far (that works) and I wanted some feedback on my developing coding style.

public static void main(String[] args) {
 Scanner sc = new Scanner(System.in);
 double numOne, numTwo, numThree, negativeB,
 discriminant, divisor, sqRtDisc, dividendPlus,
 dividendMinus;
 System.out.println("Enter the value for A: ");
 numOne = sc.nextFloat();
 System.out.println("Enter the value for B: ");
 numTwo = sc.nextFloat();
 System.out.println("Enter the value for C: ");
 numThree = sc.nextFloat();
 negativeB = -numTwo;
 discriminant = (numTwo * numTwo) - 4 * numOne * numThree;
 divisor = 2 * numOne;
 sqRtDisc = Math.sqrt(discriminant);
 System.out.println(" ");
 if (sqRtDisc > 0) {
 dividendPlus = ((negativeB + sqRtDisc) / divisor);
 dividendMinus = ((negativeB - sqRtDisc) / divisor);
 System.out.println("x = " + dividendPlus);
 System.out.println("x = " + dividendMinus);
 } else {
 System.out.println("x = " + (negativeB / divisor) + " + " 
 + (Math.sqrt(-discriminant) / divisor) + "i");
 System.out.println("x = " + (negativeB / divisor) + " - " 
 + (Math.sqrt(-discriminant) / divisor) + "i");
 System.out.println(" ");
 }
}

• PALADIN 458S: Thanks for the help, guys. I made a new version of it using what you recommended and I was surprised that I was able to cut it down to 31 lines of code from 43. Works perfectly!

Answer 1

Bugs

You are losing precision by calling sc.nextFloat() instead of sc.nextDouble().

If the equation has complex roots, Math.sqrt(discriminant) will crash.

Style

You are using too many variables to easily keep track of them mentally. Also, it's better to avoid a giant block of declarations like your

double numOne, numTwo, numThree, negativeB,
 discriminant, divisor, sqRtDisc, dividendPlus,
 dividendMinus;

We know that we need to store at least a, b, and c, so let's start there. Why not use the conventional terminology instead of numOne, numTwo, numThree?

public static void main(String[] args) {
 Scanner sc = new Scanner(System.in);
 System.out.println("Enter the value for A: ");
 double a = sc.nextDouble();
 System.out.println("Enter the value for B: ");
 double b = sc.nextDouble();
 System.out.println("Enter the value for C: ");
 double c = sc.nextDouble();

from there, the next important calculation is the discriminant, and optionally, its square root and the divisor.

 double discriminant = (b * b) - 4 * a * c;
 double divisor = 2 * a;

From there, you could go straight to formatting. I suggest System.out.printf() for readability.

 if (discriminant < 0) { // Complex roots
 System.out.printf("x = %d + %d i\n",
 -b / divisor, Math.sqrt(-discriminant) / divisor));
 System.out.printf("x = %d - %d i\n",
 -b / divisor, Math.sqrt(-discriminant) / divisor));
 } else {
 System.out.println("x = " + (-b + Math.sqrt(discriminant)) / divisor);
 System.out.println("x = " + (-b - Math.sqrt(discriminant)) / divisor);
 }
}

• Chris: One important note not explicitly called out is that you fixed the logic bug whereby the OP was testing if sqRtDisc was greater than 0 (which I would expect it always would be). You have correctly tested discriminant for being positive or negative.
• 200_success: @Chris As I wrote, "If the equation has complex roots, Math.sqrt(discriminant) will crash."
• Chris: @200_success: Indeed. I actually missed that and edited my comment to take that out (not sure if you saw it pre-edit). My point was a slightly different bug that sqRtDisc would always be positive if it didn't crash because it is the wrong variable to check. Your code does implicitly fix this bug but I thought worth pointing out to the OP in case he hadn't noticed the original bug. Its only a minor point though.

Answer 2

This is too much work for main(), and in Java, it's good to make code more reusable by utilizing methods and classes. Instead, have main() take all the input, then pass them to one or more methods that perform the calculations.

Your main() can look roughly like this:

public static void main(String[] args) {
 Scanner sc = new Scanner(System.in);
 try {
 System.out.println("Enter the value for A: ");
 double numOne = sc.nextDouble();
 System.out.println("Enter the value for B: ");
 double numTwo = sc.nextDouble();
 System.out.println("Enter the value for C: ");
 double numThree = sc.nextDouble();
 } catch (InputMismatchException e) {
 System.err.println("Invalid input given");
 // handle error here
 }
 double result = calculate(numOne, numTwo, numThree);
 sc.close();
}

You may of course add additional methods or use different names, but this should give you a general idea of how main() should look. Also notice that I've moved around the input variables. It is best to keep variables as close in scope as possible so that it'll be easier to maintain them over time.

I've also added some exception-handling for the input. You can read more about nextDouble()'s functionality in the docs. I've already provided an incomplete example of this, using the most likely exception that it may throw here. You shouldn't allow the program to continue to the calculations until valid input is given. You can either ask the user again for input, or just terminate the program.

Lastly, I've called close() to prevent leaking of resources from the Scanner object. You should always close a Scanner after its use, unless you use try-with-resources, available since Java 7.

• PALADIN 458S: If I put the calculations into another method, do I have to do anything special to call up the calculations into the answer? If a class, do I have to start out with the "extends" command?
• Jamal: @PALADIN458S: You just have to give the methods the input and have them return the final value. You may just need one method for this. In a class, you wouldn't need extends here. That's just needed if you're utilizing inheritance, but you just need helper methods.