3
\$\begingroup\$

I am new to java, and to programming as a whole. trying to become a self taught programmer. i read books and solve exercise. the issue i face is lack of feedback. any comments on this piece of code is appreciated :

/*
 * Quadratic.java
 * ------------------
 * calculates the solutions of a quadratic equation (a*x^2 + b*x + c).
 * this program calculates only real solutions. constant a is assembly nonzero.
 */

 // to use input and output tools
 import java.util.Scanner;

 public class Quadratic {
/**
 * prompt user to enter a-b-c values, computes solutions, display result
 */
public static void main(String [] args) {
    // create scanner object to read values from user
    Scanner input = new Scanner(System.in);
    // prompt user to enter quadratic equation constants values (a,b,c)
    System.out.println("Enter coefficients for the quadratic equation:");
    // enter constant 'a' value
    System.out.println("a: ");
    double a = input.nextDouble();
    // enter constant 'b' value
    System.out.println("b: ");
    double b = input.nextDouble();
    // enter constant 'c' value
    System.out.println("c: ");
    double c = input.nextDouble();
    // calculate quadratic equation solution
    quadraticEquationSolver(a,b,c);
}

/**
 * calculates quadratic equation solutions and print it to screen
 * this method calculates only real solutions, in cases where that is not 
 * the case, a message to that effect is displayed on screen.
 */
private void quadraticEquationSolver(double a, double b, double c) {
    // calculate square delta. delta = b^2 - 4*a*c
    double deltaSquare = Math.pow(b, 2) - (4 * a *c);
    // check if deltaSquare is a negative or a non-negative
    if(deltaSquare < 0) {
        // the equation has no real solutions
        System.out.println("The equation has no real solutions");
        return;
    }else {
        // calculate delat = sqrt (squareDelta)
        double delta = Math.sqrt(deltaSquare);
        // calculate first solution
        double x1 = (-b + delta) / (2 * a);
        // calculate second solution
        double x2 = (-b - delta) / (2 * a);
        // display result, each solution on different line
        System.out.println("The first solution is " + x1);
        System.out.println("The second solution is " + x2);
        // in case solutions are equal, point it out 
        if (x1 == x2) System.out.println("both solutions are equal");
    }
}
}

comments? is it too much? or just not the right way or words? return statement used in method quadraticEquationSolver, inside if statement then clause? is it ok? bad? any other alternative? if else statement, it could have been a cascaded if else statement like:

if(deltaSquare < 0) {
    // the equation has no real solutions
    System.out.println("The equation has no real solutions");
    return;
}else if (deltaSquare  == 0){
    // solutions are equal
    double solution = (-b ) / (2 * a);
    // display solution
    System.out.println("The first solution = The second solution = " + solution);
} else {
    // calculate delat = sqrt (squareDelta)
    double delta = Math.sqrt(deltaSquare);
    // calculate first solution
    double x1 = (-b + delta) / (2 * a);
    // calculate second solution
    double x2 = (-b - delta) / (2 * a);
    // display result, each solution on different line
    System.out.println("The first solution is " + x1);
    System.out.println("The second solution is " + x2);
}

which approach is better, the detailed cascading if statement ? or the brief more general simple if else statement? , in terms of good software engineering.

method return: here i faced two challenges. 1) in then clause of if else statement, method should not return any value. how to handle a situation where in its most cases the method should return a value but in one or two case, it does not return anything? 2) in other cases the solution involves two values which is not possible for a method to return two values. i got around this situation by declaring a non return method which just communicates the results to the user directly. is it too bad to do that? or is it common ?

lastly, any comments you have to improve? from any point of view

\$\endgroup\$
5
\$\begingroup\$

Code Conventions

I recommend checking out the Java Code Conventions for some style tips and also about comments. You can stick with that, but the more modern Google Java Style Guide has some good information/conventions to. There is also a good Java Doc guide. Lastly, are you using an IDE? It is really good to start without one, to learn what happens behind the scenes, but once you got that covered I would recommend switching to one. It will give you tips on comments and styles.

Comments

I would say they are a bit too much for a "professional" program (see above), but this is just an exercise, and for you it could be just right at this time. Once you have seen System.out.println a million times you no longer need to comment "display solution". :)

Program Flow

I might be a bit tired, but what do you expect the return statement to do? Will the program not behave the same without it? The if and else clauses can't be true at the same time? I.e., you can remove it.

I prefer what you call the "cascading" if statements, due to it being more clear on the different cases.

Using return and break "incorrectly" is a bit as the GOTO, it makes things confusing. However, there are times, where it would be really inconvenient to not use return or break, then it is perfectly fine to use it, see here and here. In my opinion, it is a good idea to think about the structure of a more complex flow on a piece of paper before actually implementing it.

Method Returns

It really depends on the method and your preference of Object oriented style or Procedural style for the particular problem. Even though Java is object oriented it can be preferable sometimes to use a more procedural style and use static methods, compare for example the static to non-static methods in the Integer class.

A common situation is that a method (or function) should return a positive value, then a negative value (usually -1) could signify that there was an error, or no solution.

Another situation is when you need to return more than one value. Here you can create a class and return an object of that class, containing those different values, including one which could be a boolean representing if a solution exists or not. A more "procedural" solution would be to use a void method and just update static variables in the calling class, including that boolean. If you are uncertain of the meaning of static you should read up on it, for example here.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ that's vary helpfull and full of insights. \$\endgroup\$ – ahmed dahy Jul 6 '18 at 12:09
  • \$\begingroup\$ return statement , i used it as an attempt to avoid wasting computation time on evaluating statements that won't be of any use if the condition is true. as you said in this case it is useless as the method body is entirely seplited between the two if statement clauses. but is it valid or even preffered to do so in other cases where for example there are more statements outside and under if else statement that i no longer need if certain if condition is true? \$\endgroup\$ – ahmed dahy Jul 6 '18 at 12:22
3
\$\begingroup\$
  • Comments are for explaining why, not what. The code already tells you what it's doing. Most of your comments aren't helpful.

  • You need to close resources like Scanner. Use a try-with-resources block to do so.

  • Use whitespace lines to group related code concepts together, such as prompting for a and then reading it in.

  • Use whitespace between characters consistently. It makes the code much easier to read. Not doing so is an indication of a lack of attention to detail - not a desirable trait.

  • Naming - a Solver is a noun. A method performs an action, and shouldn't have a noun name. A better name would be solveQuadraticEquation.

  • You don't need an else block if you're returning from the if.

  • If you really needed to, you could save some calculations in your code at the expense of clarity. I wouldn't suggest it here.

  • If the requirements allow it, the non-cascading code is cleaner. You should avoid the cascading if by returning early, if you prefer that approach.

  • Yes, it would be preferable to separate the IO from the business logic. This allows you to more easily swap out IO sources.

  • As far as variable-length returns, you would typically use either an array or some sort of collection. In this case, probably a double[] would be correct.

The above thoughts applied to your code might look something like:

import java.util.Scanner;

public class Quadratic {
    /**
     * prompt user to enter a-b-c values, computes solutions, display result
     */
    public static void main(final String [] args) {

        try (final Scanner input = new Scanner(System.in)) {
            System.out.println("Enter coefficients for the quadratic equation:");

            System.out.println("a: ");
            final double a = input.nextDouble();

            System.out.println("b: ");
            final double b = input.nextDouble();

            System.out.println("c: ");
            final double c = input.nextDouble();

            solveQuadraticEquation(a, b, c);
        }
    }

    /**
     * calculates quadratic equation solutions and print it to screen
     * this method calculates only real solutions, in cases where that is not
     * the case, a message to that effect is displayed on screen.
     */
    private static void solveQuadraticEquation(final double a, final double b, final double c) {
        final double deltaSquare = Math.pow(b, 2) - (4 * a * c);
        if (deltaSquare < 0) {
            System.out.println("The equation has no real solutions");
            return;
        }
        final double delta = Math.sqrt(deltaSquare);
        final double x1 = (-b + delta) / (2 * a);
        final double x2 = (-b - delta) / (2 * a);
        System.out.println("The first solution is " + x1);
        System.out.println("The second solution is " + x2);
        if (x1 == x2) {
            System.out.println("both solutions are equal");
        }
    }

    /**
     * calculates quadratic equation solutions and print it to screen
     * this method calculates only real solutions, in cases where that is not
     * the case, a message to that effect is displayed on screen.
     */
    private static void solveQuadraticEquation2(final double a, final double b, final double c) {
        final double deltaSquare = Math.pow(b, 2) - (4 * a * c);

        if (deltaSquare < 0) {
            System.out.println("The equation has no real solutions");
            return;
        }

        if (deltaSquare == 0) {
            final double solution = (-b ) / (2 * a);
            System.out.println("The first solution = The second solution = " + solution);
            return;
        }

        final double delta = Math.sqrt(deltaSquare);
        final double x1 = (-b + delta) / (2 * a);
        final double x2 = (-b - delta) / (2 * a);
        System.out.println("The first solution is " + x1);
        System.out.println("The second solution is " + x2);
    }

    public static void main2(final String [] args) {

        try (final Scanner input = new Scanner(System.in)) {
            System.out.println("Enter coefficients for the quadratic equation:");

            System.out.println("a: ");
            final double a = input.nextDouble();

            System.out.println("b: ");
            final double b = input.nextDouble();

            System.out.println("c: ");
            final double c = input.nextDouble();

            final double[] solutions = solveQuadraticEquation3(a, b, c);
            if (solutions.length == 0) {
                System.out.println("The equation has no real solutions");
            } else {
                for (int i = 0; i < solutions.length; i++) {
                    System.out.println("Solution: " + solutions[i]);
                }
            }
        }
    }

    private static double[] solveQuadraticEquation3(final double a, final double b, final double c) {
        final double deltaSquare = Math.pow(b, 2) - (4 * a * c);

        if (deltaSquare < 0) {
            return new double[0];
        }

        if (deltaSquare == 0) {
            return new double[] { (-b ) / (2 * a) };
        }

        final double[] solutions = new double[2];
        final double delta = Math.sqrt(deltaSquare);
        solutions[0] = (-b + delta) / (2 * a);
        solutions[1] = (-b - delta) / (2 * a);
        return solutions;
    }

}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ thank you for the detail. Yes, it would be preferable to separate the IO from the business logic. This allows you to more easily swap out IO sources. can you expalin more? \$\endgroup\$ – ahmed dahy Jul 6 '18 at 16:53
0
\$\begingroup\$

All of the answers deal quite well. I have only one additional point.

Assuming this would be production code you are mixing up UI with logic. There are two ways to address this:

First So you consider separation of concerns with the help of the Observer/Listener pattern. Here you can proclamate partial results any time you want.

e.g.

... 

private void quadraticEquationSolver(double a, double b, double c) {

    double deltaSquare = Math.pow(b, 2) - (4 * a *c);

    if(deltaSquare < 0) {
        notifyNoRealSolution();
    } else {

        double delta = Math.sqrt(deltaSquare);
        double x1 = (-b + delta) / (2 * a);
        notifySecondSolution(x1);

        double x2 = (-b - delta) / (2 * a);
        notifySecondSolution(x2);

        if (x1 == x2) notifyEquality(x1, x2);

    }

}

... 

Second Another possibility is that you deferr the ouput until the end so your programm follows the simple Input-Processing-Output paradigm.

InputData inputData = input();
ProcessResult processResult = process(inputData);
output(processResult);
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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