First Java program (calculator) problems

Question

I'm in the process of learning Java and my first project is a calculator, however I've run into a snag. I'm trying to get my calculator to let me enter a number then click an operator (+, -, x, /), enter another number then hit an operator again and have the display update and be able to keep this going.

Example, I would like to be able to hit the following and have it display the total each time I hit an operator after the first:

a + b / c - d =

The code I have seems (to me) like it should work but it doesn't. What am I doing wrong?

The following is the code I'm using when you hit an operator. By default wait is set to false. After running through the class once, value1 is stored and wait is set to true and that works fine. From there it doesn't seem to work quite right:

class OperatorListener implements ActionListener {
 public void actionPerformed(ActionEvent event) {
  String input = event.getActionCommand();

  // Set display as string
  String s = display.getText();

  if (!wait) {
   // Convert first input string to double
   try {
    value1 = Double.valueOf(s.trim()).doubleValue();
   } catch (NumberFormatException nfe) {
    System.out.println("NumberFormatException: " + nfe.getMessage());
   }

   dec = false;
  } else {
   // Convert second input string to double
   try {
    value2 = Double.valueOf(s.trim()).doubleValue();
   } catch (NumberFormatException nfe) {
    System.out.println("NumberFormatException: " + nfe.getMessage());
   }

   // Determine operation to be performed
   if (operator == "add") {
    value1 = Operators.add(value1, value2);    
   } else if (operator == "subtract") {
    value1 = Operators.subtract(value1, value2);
   } else if (operator == "multiply") {
    value1 = Operators.multiply(value1, value2);
   } else if (operator == "divide") {
    value1 = Operators.divide(value1, value2);
   }

   // Convert final value to string and display
   display.setText(Double.toString(value1));

   dec = false;
  }

  // Determine operator hit
  if (input.equals("+")) {
   operator = "add";
  } else if (input.equals("-")) {
   operator = "subtract";
  } else if (input.equals("x")) {
   operator = "multiply";
  } else if (input.equals("/")) {
   operator = "divide";
  }

  // Set wait
  wait = true;

 }
}

EDIT: Updated code to fix some confusion and update the if statement. Even after this the same problem still exists. Also, the full source is available here

Answer 1

Whenever you enter an operator, your code will execute this:

Double.valueOf(s.trim())

for setting either value1 or value2 (depending on wait). This will throw an exception because operators can't be parsed as doubles. You might have better luck checking for the operator first, before trying to parse the input as a number. Then if it was an operator, you can skip the number parsing part.

Also consider what might happen if somebody were to enter two numbers or two operators in a row.

Answer 2

As Greg said, no matter what the input and no matter what the current program state, you always parse out number. You need to track the program state more cleanly. I assume that when you code has "String s = output.getText();" that you really mean "String s = input.getText();".

Also note that

  if (wait == false) {
    // Stuff for !wait
  } else if (wait == true) {
    // Stuff for wait
  }

is unnecessarily redundant. You can replace it with:

  if (!wait) {
    // Stuff for !wait
  } else {
    // Stuff for wait
  }

You should probably check the input string to see if it is an operator, first, and if it isn't then make sure it is numeric. Writing an infix calculator (that properly handles precedence) is not trivial.

Answer 3

A few suggestions.

First, I would suggest when using a boolean as a conditional for an if statement, avoid comparison with true and false -- there are only two states for boolean anyway. Also, since there are only two states, rather than using else if (false), an else will suffice:

if (condition == true)
{
  // when condition is true 
}
else if (condition == false)
{
  // when condition is false
}

can be rewritten as:

if (condition)
{
  // when condition is true 
}
else
{
  // when condition is false
}

Second, rather than comparing the string literals "add", "subtract" and such, try to use constants (final variables), or enums. Doing a String comparison such as (operator == "add") is performing a check to see whether the string literal "add" and the operator variable are both refering to the same object, not whether the values are the same. So under certain circumstances, you may have the operator set to "add" but the comparison may not be true because the string literal is refering to a separate object. A simple workaround would be:

final String operatorAdd = "add";
// ...

if (input.equals("+"))
  operator = operatorAdd;
  // ...

if (operator == operatorAdd)
  // ...

Now, both the assignment of operator and the comparison of operator both are referecing the constant operatorAdd, so the comparison can use a == rather than a equals() method.

Third, as this seems like the type of calculator which doesn't really require two operands (i.e. operand1 + operand2), but rather a single operand which is acting upon a stored value (i.e. operand + currentValue), it probably would be easier to have some variable that holds the current value, and another variable that holds the operator, and a method which will act according to the current operator and operand. (More or less an idea of an accumulator machine, or 1-operand computer.)

The basic method of operation will be:

1. Set the currentValue.
2. Set the operator.
3. Set the operand.
4. Perform the calculation.
5. Set the currentValue to the result of the calculation.
6. Set the operator to blank state.

Each step should check that the previous step took place -- be sure that an operation is specified (operator is set to a valid operator), then the next value entered becomes the operand. A calculator is like a state machine, where going from one step to another must be performed in a certain order, or else it will not proceed to the next step.

So, the calculator may be like this (pseudocode!):

// Initialize calculator (Step 1)
currentValue = 0;
operand = 0;
operator = operatorNone;

loop 
{
  operand = getOperand();     // Step 2
  operator = getOperator();   // Step 3

  // Step 4 and 5
  if (operator == operatorAdd)
    currentValue += operand;
  if (operator == operatorSubtract)
    currentValue -= operand;
  // ...

  // Step 6
  operator = operatorNone;
}

Although the above code uses a single loop and doesn't work like a event-based GUI model, but it should outline the steps that it takes to run a calculator.

Answer 4

You could use the scripting engine in Java. If you don't have Java 6+, you can use Rhino which does the same thing. You can then do pretty much anything you can do in JavaScript

// create a script engine manager
ScriptEngineManager factory = new ScriptEngineManager();
// create a JavaScript engine
ScriptEngine engine = factory.getEngineByName("JavaScript");

// expose a, b, c, d
engine.put("a", 1);
engine.put("b", 8);
engine.put("c", 2);
engine.put("d", 3);

// evaluate JavaScript code from String
Number value = (Number) engine.eval("a + b / c * d");
System.out.println(value);

For more examples

Answer 5

After searching high and low I finally determined that the problem didn't lie within the code I provided. I had had a "wait = false;" in my NumberListener class that was screwing up the execution. To solve this I created 2 separate wait variables and all is working fine so far.

Thanks for the help and the tips guys, +1 to all of you for trying.