views:

1807

answers:

5

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

+3  A: 

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.

Greg Hewgill
the value of s is set in the String s = output.getText(); part. Also, if I set the operator first then it wont total the two values using the proper operator.
PHLAK
Oh, I see what you're doing, you are using the single display field for both input and output. Using "output" as the input variable name was unexpected. :)
Greg Hewgill
Yeah, I named it "output" because it's the value being output to the screen. I have updated this with "display" to help avoid confusion.
PHLAK
I think it's time for a test case. The best we have to work with is "it doesn't seem to work quite right", which as you can imagine is hard to translate into actual inputs. (1) What did you do? (2) What happened? (3) What did you expect to happen?
Greg Hewgill
+1  A: 

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.

Eddie
When I say String s = output.getText(); Im refering to a JTextField instance named "object". I guess I should change it to "display", that's a better fitting word. Also, while I agree with your if statement changes, your recommendation shouldn't make a difference in the execution am I wrong?
PHLAK
there would be no difference in the excecution, it just looks cleaner
Bedwyr Humphreys
although I would have the check for wait being true first and have the !wait code in the else part
Bedwyr Humphreys
I'm trying to keep the flow of code in the logical order of input. First the wait value would be first and you would assign a value to value1, then next time around you would assign to value2 and calculate the total. Just makes it easier to read in my opinion.
PHLAK
+4  A: 

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.

coobird
Don't you want operator.equals() instead of ==?
drhorrible
By refering to a contant in both assigning the variable and comparing the variable, using a == is valid, as they both will be refering to the same object. In the example above, using the equals() method is not necessary.
coobird
A: 

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

Peter Lawrey
Technically that works but I think it misses the point of the question - understanding the Java code - and besides, delegating simple tasks to another language (esp. Javascript) is a bad habit to get into since language boundaries are easy places for bugs to occur.
David Zaslavsky
A: 

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.

PHLAK