Basic questions

[Mike Savvy]

Hello! I wrote a little console calculator application. Would you suggest any improvements?
(1) The problem is that Eclipse saying for example for every System.out "Replace this use of System.out or System.err by a logger."
How I can replace it with logger and why is it better?
(2) Then eclipse says that I never use Object "input" - but I think I use it every loop again, so where is the problem?
(3) And third it says for "do" expression: "Reduce the total number of break and continue statements in this loop to use at most one." - I use breaks in switch cases, so why reduce?
Thank you!

package calculator;
import java.util.Scanner;

public class Calculator
{
  public static void main( String[] args )
  {
    System.out.println("Keywords: plus, minus, derive, multiply");
    System.out.println("Input example: 9 plus 3");
    Scanner input = new Scanner(System.in);
    String array[];
    do {
      System.out.print("Input: ");
      array = input.nextLine().split( " " );
      if (array[0].equals( "stop" )) {
        System.out.println("Application is closed");
        System.exit( 0 );
      }
      else if (array.length != 3) {
        System.out.println("Wrong input. \nExample: \"9 plus 3\"");
        continue;
      } 
      try {
        Double.parseDouble(array[0]);
        Double.parseDouble(array[2]);
      } catch (NumberFormatException e) {
        System.out.println("Please enter digits, not letters");
        continue;
      }
      switch (array[1]) {
        case "plus":
          System.out.println( Double.parseDouble(array[0]) + Double.parseDouble(array[2]));
          break;
        case "minus":
          System.out.println( Double.parseDouble(array[0]) - Double.parseDouble(array[2]));
          break;
        case "derive":
          System.out.println( Double.parseDouble(array[0]) / Double.parseDouble(array[2]));
          break;
        case "multiply":
          System.out.println( Double.parseDouble(array[0]) * Double.parseDouble(array[2]));
          break;
        default:
          System.out.println("Unknown operator. Available operators:\n\"plus\"\n\"minus\"\n\"derive\"\n\"multiply\"");
        }
    } while (true);
  }
}

[Campbell Ritchie]

Mike Savvy wrote:. . . . Would you suggest any improvements?

Yes. Get all that code out of the main() method. You aren't writing object‑oriented code. Where have you got a Calculator object, for example?
Don't use System.exit(). Arrange for your loop to terminate when you write, “stop”.

. . . "Replace this use of System.out or System.err by a logger."

Eclipse makes suggestions, not instructions. That may be one of several suggestions for the same code. Many people use loggers for exception messages so they have the information in a file and can correct anything in the code. The stack trace would be incomprehensible to end‑users.

. . . "Reduce the total number of break and continue statements . . ." . . .

You have written continue; twice.
The structure of that loop isn't at all good; it is an infinite loop with only System.exit() to escape it.
Nor am I happy with your exception handling. You shouldn't be doing argument validation in that loop. The keyboard input and validation belong elsewhere, probably in a utility class.
Don't mix Scanners and Double.parseDouble(). The Scanner has enough methods to get the input directly without your needing to use parseXXX() methods. What's more, the hasNextXXX() methods allow you to avoid using exceptions at all.

[Tim Holloway]

System.in, System.out, and System.err are the unholy trinity of Unix-style programs. There is absolutely nothing wrong with using them for command-line applications. That's what they were designed for, in fact.

However, not all environments are designed to work that way. Web applications, for example. For cases like that, a logger is much better because A) the System I/O classes don't officially exist in the web application paradigm and B) you get more functionality out of loggers.

A logger can do many things that simple stdio cannot. You can set logging levels on your log statements so that fine details will not ordinarily be output, but with the "flip of a switch" you can turn them on without having to make extensive program changes. This is invaluable when a program is running in production and it starts acting up.

Another thing loggers can do is routing. You can select the destination - if any - for different components of an application based on its logger ID, which is often based on the package structure of the app and its libraries (if you're using third-party libraries). For example, by setting the logging parameters correctly, you can see the SQL that Hibernate is generating for your application if you're using Hibernate. You can also do things like email and/or page people if a high-severity log message is generated such as "disk nearly full". You can log to a database or a central log manager for archival purposes.

And then there are other useful things. Like logging usually adds a timestamp and a subsystem identifier to the log messages to make it automatically more obvious when and where something happened. And you can log Exceptions with their stack traces using simple code.

You don't need (or even want!) any of this for a simple app like "input 2 numbers and output their sum", but logging is very definitely useful in many industrial-grade systems.

[salvin francis]

Mike Savvy wrote:...
(1) The problem is that Eclipse saying for example for every System.out "Replace this use of System.out or System.err by a logger."

I'd like to add that this is not a default feature in Eclipse. Rather, I think your eclipse installation has a plugin like SonarLint which does a code quality check and suggests the above action

Mike Savvy wrote:
(2) Then eclipse says that I never use Object "input" - but I think I use it every loop again, so where is the problem?

I think that the warning message is "Resource leak: 'input' is never closed". You can ignore this warning.

[Campbell Ritchie]

salvin francis wrote:. . . "Resource leak: 'input' is never closed". You can ignore this warning.

Eclipse gives you that warning, which I have seen many times, whenever you fail to close a Scanner or similar. You mustn't close a Scanner reading System.in, otherwise you close System.in. The correct option to pick is that with @SuppressWarnings....

You must however close a Scanner reading everything else.

[salvin francis]

Mike Savvy wrote:Would you suggest any improvements?...

Take out all the code from Main method

Double.parseDouble is called two times on array[0] and array[1] respectively. It can be called only once instead

"derive" should be changed to "divide"

If you want to write good Character User Interface (CUI) programs, do not expect user to type in full words as commands. Specially if you support only 4 commands

Have you taken care of division by 0 ? Your program would print "Infinity" in this case.

How is the user aware of your "stop" feature without reading this code ?

The scope of "array" exceeds it's use

Use meaningful variable names

[Mike Savvy]

Hello guys, thank you very much for the reviewing!
I have changed the program so far, even though I use only one method because I do not see reason to do more (maybe only for better readability). What would you say?

package calculator;
import java.util.Scanner;
 
public class Calculator
{ 
	void calculator() {
		System.out.println("Calculator started!\nPossible operators: +,-,*,/, "
				+ "\"stop\", calculates doubles\nInput example: 9 + 3");
	    Scanner input = new Scanner(System.in);
	    String stringArray[];
	    CALCULATOR: do {
	      System.out.print("Input: ");
	      stringArray = input.nextLine().split( " " );
	      if (stringArray[0].equals( "stop" )) {
	        System.out.println("Calculator closed.");
	        break CALCULATOR;
	      } else if (stringArray.length != 3) {
	        System.out.println("Wrong input. \nExample: \"9 / 3\"");
	        continue CALCULATOR;
	      }
	      double doubleArray[] = new double [2];
	      try {
	        doubleArray[0] = Double.parseDouble(stringArray[0]);
	        doubleArray[1] = Double.parseDouble(stringArray[2]);
	      } catch (NumberFormatException e) {
	        System.out.println("Please enter digits, not letters");
	        continue;
	      } 
	      switch (stringArray[1]) {
	        case "+":
	          System.out.println( doubleArray[0] + doubleArray[1]);
	          break;
	        case "-":
	          System.out.println( doubleArray[0] - doubleArray[1]);
	          break;
	        case "/":
	        	if (doubleArray[1]==0) {
	  	    	  System.out.println("Sorry, can not derive by zero");
	  	    	  continue CALCULATOR;
	  	      }
	          System.out.println( doubleArray[0] / doubleArray[1]);
	          break;
	        case "*":
	          System.out.println( doubleArray[0] * doubleArray[1]);
	          break;
	        default:
	          System.out.println("Unknown operator. Available operators:"
	          		+ "\n\"+\"\n\"-\"\n\"/\"\n\"*\"\nTo stop calculator please enter \"stop\"");
	        }
	    } while (true);
	}
	
  public static void main( String[] args )
  {
	System.out.println("You may use calculator by enter this code reversed: 3478");
	Scanner sc = new Scanner(System.in);
    MARK: if(8743 == sc.nextInt()) {
    	Calculator calculator = new Calculator();
        calculator.calculator();
    }
    System.out.println("Bye!");
  }
}

[Campbell Ritchie]

Mike Savvy wrote:. . . I use only one method . . .

Nonononononononono. Multiple methods are not there for readability but for reliability. We have already told you not to use the main method alone.

You might say thank you, but I think you have ignored most of the advice given, and I think that code is worse than its original version.

[Paul Clapham]

Sometimes you use just "continue" and other times you use "continue CALCULATOR". It looks to me like they both do the same thing, in which case you should use one or the other. Using both of them leads to confusion since the reader has to wonder why you used different forms which do the same thing.

However I can't tell for sure whether they do the same thing because your method is way too long and it's too hard to follow the indentation down the page. Your dismissive comment "only for readability" is just wrong -- if people can't read your code then its value is diminished. And when I say "people" I include you, since as a real-life programmer you would often have to come back to look at code which you wrote a week, or a month, or a year ago. Don't rely on remembering what the code was supposed to do, it should tell you.

So yeah, you need to break that loop up. It's doing about three different things so a few methods would improve it greatly.

[Knute Snortum]

Here's a link to a quick article about HowToFormatCode (that's a link).  If you format as you code it will make things a lot easier.

[salvin francis]

It's been a couple of days, but I thought I'd still follow up on this thread. Just wanted to point out, your new code is much worse than the original code. Look carefully at all our instructions above.