Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

###Regarding the Scanner object:

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

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

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

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.

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

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.

added 5 characters in body
Source Link
Jamal
  • 35.2k
  • 13
  • 134
  • 238

a) 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);

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

  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.

a) 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);

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

  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.

Source Link
Simon Forsberg
  • 59.7k
  • 9
  • 157
  • 311

###Regarding the Scanner object:

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

a) 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);

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

lang-java

AltStyle によって変換されたページ (->オリジナル) /