CODEWITHSUNDEEP
javacalculator
HappyCane
HappyCane

Reputation: 363

Logical error in toy program (using Switch for calculator operators)

I m building this toy program (a simple calculator with a GUI) but I can't make it work. While the compiler shows that everything is fine, when I hit "=" to get a result it shows 0.0. I suppose this is happening because:

a) the result variable is instantiated in the OperatorButtons so it's value is set to 0 once the instance is closed. If this is the case, should I put it in main section?

b) my idea to use switch in the OperatorButtons is wrong for some reason.

//Declaring Variables

      private char previousOperator='+';
      private char currentOperator=' ';
      private double number1;
      private double result=0;
      private String numInStr="";

//Setting the event handler for the number buttons 

     private void NmbrBtn(java.awt.event.ActionEvent evt) {                         
            numInStr+=evt.getActionCommand();
            number1=Double.parseDouble(numInStr);
            tfDisplay.setText(numInStr);
        }

//Setting the event handler for the Operators (where the program has to keep track of the previous and the current operator) 

     private void OperatorButtons(java.awt.event.ActionEvent evt){                                  
            previousOperator=currentOperator;        
            currentOperator=evt.getActionCommand().charAt(0);


                switch (previousOperator){
                    case '+':result+=number1;
                    case '-':result-=number1;
                    case '*':result*=number1;
                    case '/':result/=number1;
                }

                numInStr="";

        }

//Setting the handler for the "="

        private void EqualsBtn(java.awt.event.ActionEvent evt) {                           

            tfDisplay.setText(result+"");
        }

Upvotes: 0

Views: 84

Answers (1)

T.J. Crowder
T.J. Crowder

Reputation: 1074266

switch cases, in Java and nearly every other language that uses the switch construct, fall through to the one below them unless you use break. That is, if previousOperator is +, then not only does result+=number1; run, but then also result-=number1; and the other two statements after it. If previousOperator is *, then both result*=number1; and result/=number1; run.

Adding break tells the compiler you don't want that:

switch (previousOperator){
    case '+':result+=number1;break;
    case '-':result-=number1;break;
    case '*':result*=number1;break;
    case '/':result/=number1;break;
}

Subjective: Spaces and such enhance readability, consider:

switch (previousOperator){
    case '+': result += number1; break;
    case '-': result -= number1; break;
    case '*': result *= number1; break;
    case '/': result /= number1; break;
}

Note how much easier it is to see the operator being applied. If a case has more than one statement other than break (or, in my view, even if it doesn't), use a line break and indentation:

switch (previousOperator){
    case '+':
        result += number1;
        break;
    case '-':
        result -= number1;
        break;
    case '*':
        result *= number1;
        break;
    case '/':
        result /= number1;
        break;
}

...but for just one statement other than break, it's not uncommon to put them all on one line. With spaces.

Upvotes: 3

Related Questions