Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
package standardcalculator;

/**
*
* @author BSK
 */
public class calculator2 extends javax.swing.JFrame {
    private String input="";//We have nothing to display on the screen,if we dont do this creates the problem of displaying the null.
    private String firstOperand="0"; //Standard firstOperand.
    private String previousResult = "";//for storing the previous result.
    private String operation="noOperation";//initialy noOperation.
    /**
    * Creates new form calculator2
    */
    public calculator2() {
        initComponents();
    }  
    private void commonAction(String inpt){
        input += inpt;
        displayscreen.setText(input);
    }
    private boolean isFirstOperand(){
        return firstOperand.equals("0");
    }
    private void button1ActionPerformed(java.awt.event.ActionEvent evt) {                                        
        commonAction("1");
    }                                       

private void button2ActionPerformed(java.awt.event.ActionEvent evt) {                                        
    commonAction("2");
}                                       

private void button3ActionPerformed(java.awt.event.ActionEvent evt) {                                        
   commonAction("3");
}                                       

private void button4ActionPerformed(java.awt.event.ActionEvent evt) {                                        
   commonAction("4");
}                                       

private void button5ActionPerformed(java.awt.event.ActionEvent evt) {                                        
  commonAction("5");
}                                       

private void button6ActionPerformed(java.awt.event.ActionEvent evt) {                                        
    commonAction("6");
}                                       

private void button7ActionPerformed(java.awt.event.ActionEvent evt) {                                        
   commonAction("7");
}                                       

private void button8ActionPerformed(java.awt.event.ActionEvent evt) {                                        
   commonAction("8");
}                                       

private void button9ActionPerformed(java.awt.event.ActionEvent evt) {                                        
  commonAction("9");
}                                       

private void button0ActionPerformed(java.awt.event.ActionEvent evt) {                                        
   commonAction("0");
}                                       

private void buttonDotActionPerformed(java.awt.event.ActionEvent evt) {                                          
    commonAction(".");
}                                         

private void butMulActionPerformed(java.awt.event.ActionEvent evt) {                                       
    operation = "multiply";
    double result;
    input = displayscreen.getText();
    if (input.equals("")){

    }
    else if ( isFirstOperand()){
        firstOperand = input;
        previousResult = firstOperand;
        displayscreen.setText("");
    }
    else{
        result = Double.parseDouble(previousResult);
        result *= Double.parseDouble(input);
        displayscreen.setText(result+"");
        previousResult = result+"";
        firstOperand = "0";
    }
    input=""; //As we have stored the input now we need new input ,i.e,the new variable.
}                                      

private void butDivideActionPerformed(java.awt.event.ActionEvent evt) {                                          
    operation = "divide";
    double result;
    input = displayscreen.getText();
    if ( input.equals("")){

    }
    else if ( isFirstOperand()){
        firstOperand = input;
        previousResult=firstOperand;
        displayscreen.setText("");
    }
    else{
        result = Double.parseDouble(previousResult);
        result /= Double.parseDouble(input);
        displayscreen.setText(result+"");
        previousResult = result+"";
        firstOperand = "0";
    }
    input=""; //As we have stored the input now we need new input ,i.e,the new variable.
}                                         

private void butMinusActionPerformed(java.awt.event.ActionEvent evt) {                                         
    operation = "minus";
    input = displayscreen.getText();
    double result;
    if ( input.equals("")){

    }
    else if ( isFirstOperand()){
        firstOperand = input;
        previousResult=firstOperand;
        displayscreen.setText("");
    }

    else{
            result = Double.parseDouble(previousResult);
            result -= Double.parseDouble(input);
            displayscreen.setText(result+"");
            previousResult = result+"";
            firstOperand = "0";
        }
    input=""; //As we have stored the input now we need new input ,i.e,the new variable.
}                                        

private void butAddActionPerformed(java.awt.event.ActionEvent evt) {                                       
    operation = "add";
    input = displayscreen.getText();
    double result;
    if ( input.equals("")){

    }
    else if ( isFirstOperand()){
        firstOperand = input;
        previousResult=firstOperand;
        displayscreen.setText("");
    }
    else{
            result = Double.parseDouble(previousResult);
            result += Double.parseDouble(input);
            displayscreen.setText(result+"");
            previousResult = result+"";
            firstOperand = "0";
        }
    input=""; //As we have stored the input now we need new input ,i.e,the new variable.
}                                      

private void butEqualsActionPerformed(java.awt.event.ActionEvent evt) {                                          
    double result;   
    if ( input.equals("")){

    }
    else{  
        switch(operation){
            case "add":
                result = Double.parseDouble(previousResult) + Double.parseDouble(input);
                displayscreen.setText(result+"");
                break;
            case "minus":
                result = Double.parseDouble(previousResult) - Double.parseDouble(input);
                displayscreen.setText(result+"");
                firstOperand="0";
                break;
            case "multiply":
                result = Double.parseDouble(previousResult) * Double.parseDouble(input);
                displayscreen.setText(result+"");
                firstOperand="0";
                break;
            case "divide":
                result = Double.parseDouble(previousResult) / Double.parseDouble(input);
                displayscreen.setText(result+"");
                firstOperand="0";
                break;
            case "modulus":
                result = Double.parseDouble(previousResult) % Double.parseDouble(input);
                displayscreen.setText(result+"");
                firstOperand="0";
                break;
            case "reciprocal":
                result = 1/Double.parseDouble(previousResult);
                displayscreen.setText(result+"");
                firstOperand="0";
                break;
            case "root":
                result = Math.sqrt(Double.parseDouble(previousResult));
                displayscreen.setText(result+"");
                firstOperand="0"; 
                break;  
            case "noOperation":
                break;
       }
    }
}                                         

private void displayscreenActionPerformed(java.awt.event.ActionEvent evt) {                                              
    //input=displayscreen.getText();
}                                             

/**
 * @param args the command line arguments
 */
public static void main(String args[]) {
     java.awt.EventQueue.invokeLater(new Runnable() {
        public void run() {
            new calculator2().setVisible(true);
        }
    });
}

// Variables declaration - do not modify                     
private javax.swing.JButton butAdd;
private javax.swing.JButton butDivide;
private javax.swing.JButton butEquals;
private javax.swing.JButton butMinus;
private javax.swing.JButton butModulus;
private javax.swing.JButton butMul;
private javax.swing.JButton butReciprocal;
private javax.swing.JButton button0;
private javax.swing.JButton button1;
private javax.swing.JButton button2;
private javax.swing.JButton button3;
private javax.swing.JButton button4;
private javax.swing.JButton button5;
private javax.swing.JButton button6;
private javax.swing.JButton button7;
private javax.swing.JButton button8;
private javax.swing.JButton button9;
private javax.swing.JButton buttonDot;
private javax.swing.JTextField displayscreen;
// End of variables declaration
}

Here are the conditions that I have covered:

  • The multiply button is pressed again and again. If we have not input and pressing the multiply button.
  • Repeatedly multiply without pressing the "=" key. Note: Input is given and shown in textField each time user enter a button, I did not show the code for that.

My question is about the readability of the code, and my applied algorithm. Is it efficient?

Please keep patience as I am new to Java and learning to design the algorithm, and please provide me some references about how to design an efficient algorithm.

share|improve this question
    
Would you consider using Java 8? Just for my suggestions... –  Boris the Spider Jun 24 at 16:46
    
@BoristheSpider Sorry!whats nice about that one. –  Bayant_singh Jun 24 at 16:52

3 Answers 3

Step 1 : Reduce different event handlers which performs almost same task.

How:

Step 1.1: The button click events (button1ActionPerformed and similar others) which are calling the commonAction method, delete all except 1 method.

Step 1.2: Analyze the event variable of the event you kept. It should have information about the button, get the text of the button and pass it as a parameter of commonAction method

step 1.3: For the other button's click events, attach the same event for all of them.

share|improve this answer
public class calculator2 extends javax.swing.JFrame {

The class names should be UpperCamelCase, so it will be Calculator2.

Why Calculator 2? What means this two? Maybe you should choose another name or just Calculator.

javax.swing.

You can avoid to rewrite the full package using import

private String input="";//We have nothing to display on the screen,if we dont do this creates the problem of displaying the null.
private String firstOperand="0"; //Standard firstOperand.
private String previousResult = "";//for storing the previous result.
private String operation="noOperation";//initialy noOperation.

I dubt they need all to be Strings. Why operation operation is a String? With your code you force it to be Java7+ only while you could use an enum with all operations and make your code looks better, or use a char so if your calculation have button "+" you can put this value directly inside the variable (but an enum is more flexible and allow you to do more edits), with strings you could write "multiply" wrong and it will not work.

Instead of noOperation string what about null ?

firstOperand
previousResult

They work like double but they are Strings, why? You need to convert it to double everytime, use double!

Inside butMultiActionPerfomed:

if (input.equals("")){

}

If you don't need this block why you do it?

If you don't want to execute the code if input is empty, change your code to:

private void butMulActionPerformed(java.awt.event.ActionEvent evt) {                                       
operation = "multiply";
double result;
input = displayscreen.getText();
if (input.isEmpty()) {
  return;
}

So if the input is empty it will block the execution of the method.

The same for other methods.

You should avoid to avoid to repeat code, all this methods butDivideActionPerformed/butMulActionPerformed/butMinusActionPerformed/butAddActionPerformed do the same thing.

You should change your code to use a method which will perform the change of the operation variable.

In the same method this block

else{
  result = Double.parseDouble(previousResult);
  result -= Double.parseDouble(input);
  displayscreen.setText(result+"");
  previousResult = result+"";
  firstOperand = "0";
}

It will be different based on the current action. You can avoid it and use the same method which is called when = is called.

butEqualsActionPerformed

Same for others. The code which will perform the operation should be inside another method so you can call it inside the other methods add, min etc. and avoid to repeat the code.

About the events which will add the numbers etc. i don't know how it's done in Java but in Android i would set a tag to the Button which contains what it should add and add one onClick to all which will read this tag to understand what value should be added. Check if ActionEvent can carry with it some custom objects.


It's how you could edit your code

private void executeAction(String operation) {
    // save operation
    this.operation = operation;

    // it's empty, so ignore it
    if (!input.isEmpty()) {
        return;
    }

    if (isFirstOperand()) {
        firstOperand = input;
        previousResult = firstOperand;
        displayscreen.setText("");
    }
    else {
        executeCalculation();
    }

    input = "";
}

executeAction will be called by your butMulActionPerformed/butDivideActionPerformed etc. example:

private void butDivideActionPerformed(java.awt.event.ActionEvent evt) {
    executeAction("divide");
}

executeCalculation is the method which will be called when you need to execute the operation.

private double executeCalculation() {
    double result = 0;
    switch(operation){
        case "add":
            result = Double.parseDouble(previousResult) + Double.parseDouble(input);
            break;
        case "minus":
            result = Double.parseDouble(previousResult) - Double.parseDouble(input);
            break;
        case "multiply":
            result = Double.parseDouble(previousResult) * Double.parseDouble(input);
            break;
        case "divide":
            result = Double.parseDouble(previousResult) / Double.parseDouble(input);
            break;
        case "modulus":
            result = Double.parseDouble(previousResult) % Double.parseDouble(input);
            break;
        case "reciprocal":
            result = 1/Double.parseDouble(previousResult);
            break;
        case "root":
            result = Math.sqrt(Double.parseDouble(previousResult));
            break;
        case "noOperation":
            break;
    }

    firstOperand="0";
    return result;
}

and your butEqualsActionPerformed will be

private void butEqualsActionPerformed(java.awt.event.ActionEvent evt) {
    if (!input.isEmpty()) {
        return;
    }

    double result = executeCalculation();
    displayscreen.setText(String.valueOf(result));
}

p.s other things you could improve: why executeAction should do displayscreen.setText(""); ?

Remember to follow Java naming style! displayscreen should be displayScreen!

share|improve this answer
    
How can i have common method for butDivideActionPerformed/butMulActionPerformed/butMinusActionPerformed/butAddAc‌​tionPerf when = is pressed?I want to take into account that case in which user adds two numbers and after that,without pressing =,he pushes + for adding another value and repeat it multiple times and i want to show each result on screen.Problem is that i want to display the result without pressing = if we have add after add operation. –  Bayant_singh Jun 24 at 16:14
    
i can take all my instances as double but i choose String as display is in the form of String –  Bayant_singh Jun 24 at 16:15
    
You can convert a double to String. I will edit with an example to how to improve the methods –  Marco Acierno Jun 24 at 16:21
    
Done, see my edit. –  Marco Acierno Jun 24 at 16:33
    
Thanx in advance in making my life easier and please also mention about the idea of enum and char.How enum is flexible? –  Bayant_singh Jun 24 at 16:36

N.B. these suggestions require Java 8. Although they can be used in earlier Java versions by using interfaces to implement the Strategy pattern - Effective Java item 21.

The first thing to note is that your are constantly turning String into double and back. Just use double throughout for your numeric values.

You have a lot of duplicate code. Most of your code is the same thing copy/pasted with a few things changed.

Lets start with the number buttons - there are ten that are exactly the same. Start by changing the commonAction method to something like this:

private void pressNumber(double number) {
    input += number;
    displayscreen.setText(input);
}

Now, using Java 8 lambdas we can rewrite the creation all the buttons to this:

private List<JButton> createNumberButtons() {
    final List<JButton> numbers = new LinkedList<>();
    for (int i = 0; i < 10; ++i) {
        final int num = i;
        final JButton number = new JButton(Integer.toString(num));
        number.addActionListener(e -> pressNumber(num));
        numbers.add(number);
    }
    return numbers;
}

You can now assign the buttons in the List to your original variables, or change your GUI to use the new List. Each button calls pressNumber with its index i. This means you can remove all of the button action methods.

Now onto your operator. Don't use a String, use an enum. This is exactly the sort of thing they were intended for. Using Java 8 function interfaces and some more lambdas we have this, rather neat, impl:

private static enum Op implements DoubleBinaryOperator {
    PLUS(Double::sum),
    MINUS((a, b) -> a - b),
    TIMES((a, b) -> a * b),
    DIVIDE((a, b) -> a / b),
    MODULO((a, b) -> a % b),
    RECIPROCAL((a, b) -> 1 / a),
    SQRT((a, b) -> Math.sqrt(a));

    private final DoubleBinaryOperator op;

    Op(final DoubleBinaryOperator op) {
        this.op = op;
    }

    @Override
    public double applyAsDouble(final double left, final double right) {
        return op.applyAsDouble(left, right);
    }
}

Now we need to a common, "operator" method that applies the selected Op to the numbers. Something along these lines ought to do:

private void operatorButtonAction(final Op op) {
    operation = op;
    if (displayscreen.getValue() == null)
        return;
    if (isFirstOperand()) {
        firstOperand = Double.parseDouble(input);
        previousResult = firstOperand;
        displayscreen.setText("");
    } else {
        final double result = op.applyAsDouble(previousResult, Double.parseDouble(input));
        displayscreen.setValue(result);
        previousResult = result;
        firstOperand = 0;
    }
    //As we have stored the input now we need new input ,i.e,the new variable.
    input = "";
}

So rather than hardcoding the selected operator, we have parametrised it out. Again, this is a strategy pattern.

Now your operator button methods look like:

private void butMulActionPerformed(ActionEvent evt) {
    operatorButtonAction(Op.TIMES);
}

In short, you need to think much more about what is common across the code. Not what is different. Try and apply design patterns to your code to clean it up. This example is predominantly using the Strategy Pattern.

share|improve this answer
    
:Sorry i am new to java,can i directly start with java8? –  Bayant_singh Jun 25 at 7:56
    
@Bayant_singh Java 8 is the newest version of Java; hot off the presses almost. If you're learning Java, it's probably be to learn Java 8 and it significantly enhances Java's capabilities. –  Boris the Spider Jun 25 at 8:19
    
Can you please suggest me some good source,a good book will be appriciated.thank you for your support. –  Bayant_singh Jun 25 at 17:26
    
@Bayant_singh The Oracle tutorials are always a good place to start. There is also a book from Manning. –  Boris the Spider Jun 26 at 6:24
    
thanx a lot for your valuable thoughts –  Bayant_singh Jun 26 at 16:59

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.