4
\$\begingroup\$

I'm new to Java and this is my first program. I'm trying to verify that my thought processes are good.

Let me know of any improvements infor the code, whether it's too few/too many comments, poorly structured code, etc.

import javax.swing.*;
import java.math.*;
import java.util.*;

class Root
{
public static void main(String[] args)
{
    //Variable & Constant Declaration
    double coeffA=0.0; //Variable for Coefficient A
    double coeffB=0.0; //Variable for Coefficient B
    double coeffC=0.0; //Variable for Coefficient C
    double xroot1=0.0; //Variable for the first root
    double xroot2=0.0; //Variable for the second root
    double discriminant=0.0; //Varaible for the discriminant
    double root1complex=0.0; //Variable for part a of the complex root 
    double root2complex=0.0; //Variable for part b of the complex root
    boolean contloop=true; //Variable for continuing loop
    String loop="y"; //Variable for input choice of continuing loop

    Scanner sc = new Scanner(System.in); //An object to read from the keyboard

    try{
        do{

            if (loop.equals("y")){

                //Input
                System.out.println("Please enter the value for the first coefficient: "); //Asks the user to input the value for the A term
                coeffA=sc.nextDouble(); //Captures the keyboard input and stores it to the variable coeffA

                System.out.println("Please enter the value for the second coefficient: "); //Asks the user to input the value for the B term
                coeffB=sc.nextDouble(); //Captures the keyboard input and stores it to the variable coeffB

                System.out.println("Please enter the value for the third coefficient: "); //Asks the user to input the value for the C term
                coeffC=sc.nextDouble(); //Captures the keyboard input and stores it to the variable coeffC


                //Calculations
                discriminant=coeffB*coeffB-(4*coeffA*coeffC); //Calculates the discriminant
                xroot1=(-coeffB+(Math.sqrt(discriminant)))/(2*coeffA); //Calculates the first root of the polynomial
                xroot2=(-coeffB-(Math.sqrt(discriminant)))/(2*coeffA); //Calculates the second root of the polynomial

                //Output
                System.out.println("The Equation is:"); //Displays the value for Coefficient C
                System.out.println(coeffA+"x^2 + "+coeffB+"x + "+ coeffC); //Displays the equation to the user
                System.out.println("The discriminant is: " + discriminant); //Displays the discriminant to the user

                    if(discriminant<0){
                        root1complex=-coeffB/(2*coeffA); //Calculates the first part of the complex root
                        root2complex=Math.sqrt(-discriminant)/(2*coeffA); //Calculates the second part of the complex root
                        System.out.println("The Equation has Imaginary roots."); //Displays to the user that there are imaginary roots
                        System.out.println("The Roots are: " + root1complex + "+" + root2complex + "i" + " and " + root1complex + "-" + root2complex + "i.");
                        if (coeffA==0){
                            System.out.println("*NaN stands for \"Not a Number.\"*");
                        } //Ends If
                    } //Ends If

                    else if(discriminant==0){
                        System.out.println("The Root is: " + xroot1 + "."); //Displays the root
                        if (coeffA==0){
                            System.out.println("*NaN stands for \"Not a Number.\"*");
                        } //Ends If
                    } //Ends If

                    else if (discriminant>0){
                        System.out.println("The Roots are: " + xroot1 + " and " + xroot2 +"."); //Displays the roots
                        if (coeffA==0){
                            System.out.println("*NaN stands for \"Not a Number.\"*");
                        } //Ends If
                    } //Ends If

                    System.out.println("Do you wish to perform another calculation?"); //Asks the user if they would like to perform another calculation
                    loop=sc.nextLine(); //Captures the next string input and stores it to the variable loop
                } //Ends If

            else if (loop.equals("n")){
                System.out.println("Quit."); //Prints "Quit"
                contloop=false; // Sets variable contloop to false thereby allowing therby allowing the program to exit the loop and finish
            } //Ends If

            else if ((!loop.equals("y")) || (!loop.equals("n"))){
                System.out.println("Please enter \"y\" or \"n\"."); //Requests the user to enter a valid input
                loop=sc.nextLine(); //Captures the next string input and stores it to the variable loop
            } //Ends If
        }while(contloop==true); //Ends Loop
        System.exit(0); //Exits Java
    } //End Try

    catch (InputMismatchException e){
        System.out.println("Invalid input format\nOnly numbers (decimals or intergers) are valid input formats.\nPlease restart program.");
    } //End Catch
} //End Main
} //End Class
\$\endgroup\$
2
  • 4
    \$\begingroup\$ looks spaghettic. Not reusable. Merging interactive IO with the calculation (business logic). \$\endgroup\$ Feb 29, 2012 at 22:10
  • \$\begingroup\$ Consider using BigDecimal instead of double. \$\endgroup\$ Mar 2, 2012 at 18:14

5 Answers 5

5
\$\begingroup\$

I agree to everything, Matthew Flaschen wrote, but like to add some more things:

You don't declare and initialize your variables with dummy values upfront. Instead, you restrict the scope as much as possible, and delay the initialization, if possible, to first usage.

In your example:

public static void main (String [] args)
{   
    boolean contloop = true; 
    String loop = "y"; 

    Scanner sc = new Scanner (System.in); 

    try {
    do {
        if (loop.equals ("y")) {

            System.out.println ("Please enter the value for the first coefficient: "); 
            double coeffA = sc.nextDouble (); 

            System.out.println ("Please enter the value for the second coefficient: "); 
            double coeffB = sc.nextDouble (); 

            System.out.println ("Please enter the value for the third coefficient: "); 
            double coeffC = sc.nextDouble (); 

            double discriminant = coeffB * coeffB - (4 * coeffA * coeffC); 
            double xroot1 = (-coeffB + (Math.sqrt (discriminant))) / (2 * coeffA); 
            double xroot2 = (-coeffB - (Math.sqrt (discriminant))) / (2 * coeffA); 

This makes reasoning about what the variable is used for, who is influenced by modifying it and refactoring much more easy.

I would suggest much more white space between the tokens. It's better readable for the human eye. The machine will not mention it. Compare yourself:

xroot1 = (-coeffB + (Math.sqrt (discriminant))) / (2 * coeffA); 
xroot1=(-coeffB+(Math.sqrt(discriminant)))/(2*coeffA); 

If you use comments, put them infront of what is commented, so that javadoc maps them correctly.

Avoid System.exit(0); //Exits Java! Your code can't be called from another Java program, if you use it, because, as written in your comment, it doesn't just stop your program, but the whole JVM. But you will often see it in tutorials.

In your case, a return; is all you need.

Trying to refactor your code, so that we would have a main loop with 3 steps:

loop 
    read input
    calculation 
    format output
while user ok

with 3 method calls, and a class for the values, I mentioned, that the control is surprisingly complicated for the task, which is only whether there should be another loop or not. But we have a variable loop, another one contloop. Can they be out of sync? Why do we have two?

But the main aspect is, to split the task into 3 smaller parts. Think of the possibility to reuse the calculation in a webinterface, Where you don't use System.out.

\$\endgroup\$
7
  • \$\begingroup\$ Thanks! Like I stated, it's my first time. In most examples I had seen on the web the variables were initialized at the point of usage. My teacher for some has continually shown us up front and wants our code formatted like that when printed. \$\endgroup\$
    – Caleb
    Feb 29, 2012 at 23:01
  • \$\begingroup\$ Also: Restrict your try ... catch to only the code that can actually throw the execeptions. \$\endgroup\$ Mar 1, 2012 at 0:20
  • \$\begingroup\$ If you want to use Javadoc, you also need to use Javadoc comments. This means specific tags within /** ... */. \$\endgroup\$ Mar 1, 2012 at 5:51
  • \$\begingroup\$ @Eagle: I can't discuss with your teacher in this thread. He is welcome to visit this site and argue his viewpoint, but we can't discuss his authority in this question without him being here. Ask your teacher why he wants the variables up front. \$\endgroup\$ Mar 1, 2012 at 8:00
  • \$\begingroup\$ @user unknown: I think it's just a preference for assignments on her part. And unless I get a really poor reason from her it's not worth talking about. I don't want to appear to disrespect her even on a website she'll never be on. I was in a rush in my last comment because the fire alarm went off as I was typing, so I couldn't make myself clear. :) \$\endgroup\$
    – Caleb
    Mar 7, 2012 at 17:56
7
\$\begingroup\$

You have way too many comments. Assume the developer reading your code knows Java, and can read standard library documentation. Thus, you do not need comments like:

//Variable for Coefficient A (or equivalent for any other variable)

//Variable & Constant Declaration

//An object to read from the keyboard (clear from standard library docs)

You also don't need the end comments:

//Ends Loop

for the same reason. It's an essential part of the language syntax, and many editors let you toggle between the start and end brace.

You have some small repetition, which you can factor out to a variable or method, like the part about NaN.

A more realistic comment is something like:

// Quadratic formula from Optimized Math Algorithms p. 46

This is silly here, since the quadratic formula is well known. But it would be useful for more novel algorithms.

Also, don't repeat calculations like Math.sqrt(discriminant) or 2*coeffA. Use an intermediate variable.

I would also separate the input, calculation, and output phases into different methods.

\$\endgroup\$
2
  • \$\begingroup\$ Hey, I'm really very new to this. I pretty much made this off of googling questions and reading the ide. I'm fuzzy in terminology. Could you give me an example of how I would separate the input, calculation, etc. into different methods? Do you mean something like this: public output(){ } \$\endgroup\$
    – Caleb
    Mar 7, 2012 at 17:33
  • 1
    \$\begingroup\$ @Caleb, yes, more or less. You'll need the output to take the values to print as arguments. The input method should return the user's input. \$\endgroup\$ Mar 7, 2012 at 19:38
4
\$\begingroup\$

Here is my take:

import java.util.Scanner;

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

    public static double askForCoefficient(String ordinal) {
        while (true) {
            try {
                System.out.println("Please enter the value for the " + ordinal + " coefficient: ");
                String s = sc.nextLine();
                return Double.parseDouble(s);
            } catch (NumberFormatException ex) {
                System.out.println("Invalid input. Please enter a number (decimal or interger)");
            }
        }
    }

    public static double[] askForCoefficients() {
        double a = 0.0;
        while (a == 0.0) {
            a = askForCoefficient("first");
            if (a == 0.0) {
                System.out.println("The first coefficient can't be 0 in a quadratic polynom.");
            }
        }
        double b = askForCoefficient("second");
        double c = askForCoefficient("third");
        return new double[]{a, b, c};
    }

    public static boolean yesOrNo(String answer) {
        return answer.equalsIgnoreCase("y") || answer.equalsIgnoreCase("n");
    }

    public static boolean askForContinue() {
        String loop = "";
        System.out.println("Do you wish to perform another calculation (y/n)?");
        while (!yesOrNo(loop)) {
            loop = sc.nextLine();
            if (!yesOrNo(loop)) {
                System.out.println("Please enter \"y\" or \"n\".");
            }
        }
        return loop.equalsIgnoreCase("y");
    }

    public static Complex[] calculate(double a, double b, double c) {
        double discriminant = b * b - (4 * a * c);
        System.out.println("The discriminant is " + discriminant);
        Complex root1 = new Complex(-b, 0).plus(Complex.sqrt(discriminant)).multReal(0.5 / a);
        Complex root2 = new Complex(-b, 0).minus(Complex.sqrt(discriminant)).multReal(0.5 / a);
        return (root1.equals(root2)) ? new Complex[]{root1} : new Complex[]{root1, root2};
    }


    public static void main(String[] args) {
        do {
            double[] coeffs = askForCoefficients();
            System.out.println("The Equation is:");
            System.out.println(coeffs[0] + "x^2 + " + coeffs[1] + "x + " + coeffs[2]);

            Complex[] roots = calculate(coeffs[0], coeffs[1], coeffs[2]);

            if (roots.length == 1) {
                System.out.println("The Root is " + roots[0] + ".");
            } else {
                if (!roots[0].isReal()) {
                    System.out.println("The Equation has imaginary roots.");
                }
                System.out.println("The Roots are: " + roots[0] + " and " + roots[1]);
            }
        } while (askForContinue());
        System.out.println("Quit.");
    }

    public static class Complex {
        public final double real;
        public final double imag;

        public Complex(double real, double imag) {
            this.real = real;
            this.imag = imag;
        }

        public Complex plus(Complex that) {
            return new Complex(this.real + that.real, this.imag + that.imag);
        }

        public Complex minus(Complex that) {
            return new Complex(this.real - that.real, this.imag - that.imag);
        }

        public boolean isReal() {
            return imag == 0.0;
        }

        public Complex multReal(double d) {
            return new Complex(d * real, d * imag);
        }

        public static Complex sqrt(double d) {
            return d >= 0.0
                    ? new Complex(Math.sqrt(d), 0.0)
                    : new Complex(0.0, Math.sqrt(-d));
        }

        public String toString() {
            if (imag == 0.0) return "" + real;
            else if (imag > 0) return real + "+" + imag + "i";
            else return real + "" + imag + "i";
        }

        public boolean equals(Object o) {
            Complex that = (Complex) o;
            return this.real == that.real && this.imag == that.imag;
        }

    }

} 

I broke the class in several methods with clear, testable sub-tasks. Most notably things got cleaner and easier when I used the right data type, which is here Complex. Don't stick with the standard classes / primitives if they don't really fit. Of course if you want to reuse and extend Complex, you should make it a top level class.

\$\endgroup\$
2
\$\begingroup\$

The last condition statement is definitely wrong:

else if ((!loop.equals("y")) || (!loop.equals("n")))

It should be &&, not ||.

Why choose a do while conditional statement? If you just wanna make sure it will be run at least one time, use a while loop.

The main function are is large; split it to small function. You will maybe find that some code can be reused.

For a comment like this:

Scanner sc = new Scanner(System.in); //An object to read from the keyboard

Everyone knows what this code does, so no need to comment. You comment simple code which already explains itself, then you repeat yourself. Remember the 'DRY' rule; don't let comments repeat yourself.

I don't quite understand the algorithmic part. Maybe you can add one MathUtil class to help you do the algorithm.

\$\endgroup\$
1
\$\begingroup\$

For a beginning programming class it doesn't matter, but a good principle is to separate the part of your code that do calculation from the parts that do input and output. The "Calculations" block should be in a separate method.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks! I'll definitely try to do that for my next program. Things kinda blended together when I got going... :D Pretty much I googled things for answers because my teacher, like so many people's, melts my mind. Plus, she doesn't teach fast enough for my tastes. I was homeschooled so I mainly look up how to do things and learn from examples before she teaches it. So my code is going to look poor for a little while. And beginning or not, it should be perfect. Because, well it just should be. :) \$\endgroup\$
    – Caleb
    Mar 7, 2012 at 17:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.