10
\$\begingroup\$

I'm very new to Java and programming and I've written a calculator to put all the theory I've learned so far into practice. I'm sure there is a lot of room for improvement since I really just started learning. Any big mistakes you more experienced programmers notice immediately?

I'm especially intrigued in how you can separate this class into several pieces so that the code is more OO. Also, what about my actionPerformed method? It looks so long and clunky.

I guess positioning all the buttons myself isn't good practice either and I should use some sort of layout like a GridLayout, but I purposely chose not to since I'm not sure whether Swing is really worth spending time on (I hear JavaFx is becoming more important).

import java.awt.EventQueue;
import java.awt.Font;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.JTextField;
import javax.swing.SwingConstants;
import javax.swing.border.EmptyBorder;

public class Calculator extends JFrame implements ActionListener
{

    private static final long serialVersionUID = 1L;

    private JPanel contentPane;
    private JTextField textField = new JTextField();

    private JButton btnOne = new JButton("1");
    private JButton btnTwo = new JButton("2");
    private JButton btnThree = new JButton("3");
    private JButton btnPlus = new JButton("+");
    private JButton btnFour = new JButton("4");
    private JButton btnFive = new JButton("5");
    private JButton btnSix = new JButton("6");
    private JButton btnMinus = new JButton("-");
    private JButton btnSeven = new JButton("7");
    private JButton btnEight = new JButton("8");
    private JButton btnNine = new JButton("9");
    private JButton btnStar = new JButton("*");
    private JButton btnPoint = new JButton(".");
    private JButton btnZero = new JButton("0");
    private JButton btnEquals = new JButton("=");
    private JButton btnSlash = new JButton("/");
    private JButton btnBack = new JButton("Back");
    private JButton btnClear = new JButton("Clear");

    private String textFieldInput;
    private JButton[] myButtons =
    { btnOne, btnTwo, btnThree, btnPlus, btnFour, btnFive, btnSix, btnMinus,
        btnSeven, btnEight, btnNine, btnStar, btnPoint, btnZero, btnEquals,
        btnSlash, btnClear, btnBack };



    /**
     * Launch the application.
     */
    public static void main(String[] args)
    {
        EventQueue.invokeLater(new Runnable()
        {
            @Override
            public void run()
            {
                try
                {
                    Calculator frame = new Calculator();
                    frame.setVisible(true);
                }
                catch (Exception e)
                {
                    e.printStackTrace();
                }
            }
        });
    }

    /**
     * Create the frame.
     */
    public Calculator()
    {
        setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        setBounds(100, 100, 450, 300);
        setSize(500, 400);
        setLocationRelativeTo(null);
        setResizable(false);

        contentPane = new JPanel();
        contentPane.setBorder(new EmptyBorder(5, 5, 5, 5));
        setContentPane(contentPane);
        contentPane.setLayout(null);

        positionTextField();
        positionButtons();

        for (JButton jButton : myButtons)
        {
            jButton.addActionListener(this);
            contentPane.add(jButton);
        }

    }

    public void positionTextField()
    {
        textField.setFont(new Font("Dialog", Font.PLAIN, 20));
        textField.setHorizontalAlignment(SwingConstants.CENTER);
        textField.setBounds(46, 12, 380, 42);
        contentPane.add(textField);
        textField.setColumns(10);
    }

    public void positionButtons()
    {
        btnOne.setBounds(46, 79, 60, 31);
        btnTwo.setBounds(139, 79, 60, 31);
        btnThree.setBounds(230, 79, 60, 31);
        btnPlus.setBounds(340, 79, 60, 31);
        btnFour.setBounds(46, 131, 60, 31);
        btnFive.setBounds(139, 131, 60, 31);
        btnSix.setBounds(230, 131, 60, 31);
        btnMinus.setBounds(340, 131, 60, 31);
        btnSeven.setBounds(46, 185, 60, 31);
        btnEight.setBounds(139, 185, 60, 31);
        btnNine.setBounds(230, 185, 60, 31);
        btnStar.setBounds(340, 185, 60, 31);
        btnPoint.setBounds(46, 231, 60, 31);
        btnZero.setBounds(139, 231, 60, 31);
        btnEquals.setBounds(230, 231, 60, 31);
        btnSlash.setBounds(340, 231, 60, 31);
        btnClear.setBounds(37, 292, 90, 48);
        btnBack.setBounds(139, 292, 90, 48);
    }

    public void setTextTo(String text)
    {
        textField.setText(textField.getText() + text);
        textField.requestFocus();
    }

    public void goBack()
    {
        if (!textField.getText().equals(""))
        {
            String text = textField.getText();
            String withoutLastChar = text.substring(0, text.length() - 1);
            textField.setText(withoutLastChar);
            textField.requestFocus();
        }
    }

    public void add()
    {
        int place = textFieldInput.indexOf("+");
        String firstChars = textFieldInput.substring(0, place);
        String lastChars = textFieldInput.substring(place + 1,
                textFieldInput.length());
        double firstCharsInt = Double.parseDouble(firstChars);
        double lastCharsInt = Double.parseDouble(lastChars);
        double result = firstCharsInt + lastCharsInt;
        String stringResult = Double.toString(result);
        textField.setText(stringResult);
    }

    public void substract()
    {
        int place = textFieldInput.indexOf("-");
        String firstChars = textFieldInput.substring(0, place);
        String lastChars = textFieldInput.substring(place + 1,
                textFieldInput.length());
        double firstCharsInt = Double.parseDouble(firstChars);
        double lastCharsInt = Double.parseDouble(lastChars);
        double result = firstCharsInt - lastCharsInt;
        String stringResult = Double.toString(result);
        textField.setText(stringResult);
    }

    public void multiply()
    {
        int place = textFieldInput.indexOf("*");
        String firstChars = textFieldInput.substring(0, place);
        String lastChars = textFieldInput.substring(place + 1,
                textFieldInput.length());
        double firstCharsInt = Double.parseDouble(firstChars);
        double lastCharsInt = Double.parseDouble(lastChars);
        double result = firstCharsInt * lastCharsInt;
        String stringResult = Double.toString(result);
        textField.setText(stringResult);
    }

    public void divide()
    {
        int place = textFieldInput.indexOf("/");
        String firstChars = textFieldInput.substring(0, place);
        String lastChars = textFieldInput.substring(place + 1,
                textFieldInput.length());
        double firstCharsInt = Double.parseDouble(firstChars);
        double lastCharsInt = Double.parseDouble(lastChars);
        double result = firstCharsInt / lastCharsInt;
        String stringResult = Double.toString(result);
        textField.setText(stringResult);
    }

    @Override
    public void actionPerformed(ActionEvent e)
    {
        if (e.getSource() == btnOne)
        {
            setTextTo("1");
        }
        else if (e.getSource() == btnTwo)
        {
            setTextTo("2");
        }
        else if (e.getSource() == btnThree)
        {
            setTextTo("3");
        }
        else if (e.getSource() == btnPlus)
        {
            if (!textField.getText().contains("+"))
            {
                setTextTo("+");
            }
        }
        else if (e.getSource() == btnFour)
        {
            setTextTo("4");
        }
        else if (e.getSource() == btnFive)
        {
            setTextTo("5");
        }
        else if (e.getSource() == btnSix)
        {
            setTextTo("6");
        }
        else if (e.getSource() == btnMinus)
        {
            if (!textField.getText().contains("-"))
            {
                setTextTo("-");
            }
        }
        else if (e.getSource() == btnSeven)
        {
            setTextTo("7");
        }
        else if (e.getSource() == btnEight)
        {
            setTextTo("8");
        }
        else if (e.getSource() == btnNine)
        {
            setTextTo("9");
        }
        else if (e.getSource() == btnStar)
        {
            if (!textField.getText().contains("*"))
            {
                setTextTo("*");
            }
        }
        else if (e.getSource() == btnPoint)
        {
            if (!textField.getText().contains("."))
            {
                setTextTo(".");
            }
        }
        else if (e.getSource() == btnZero)
        {
            setTextTo("0");
        }
        else if (e.getSource() == btnEquals)
        {
            try
            {
                textFieldInput = textField.getText();

                if (textFieldInput.contains("+"))
                {
                    add();
                }
                else if (textFieldInput.contains("-"))
                {
                    substract();
                }
                else if (textFieldInput.contains("*"))
                {
                    multiply();
                }
                else if (textFieldInput.contains("/"))
                {
                    divide();
                }
                textField.requestFocus();
            }
            catch (Exception e2)
            {
                textField.setText("Invalid operation");
            }
        }
        else if (e.getSource() == btnSlash)
        {
            if (!textField.getText().contains("/"))
            {
                setTextTo("/");
            }
        }
        else if (e.getSource() == btnClear)
        {
            textField.setText("");
            textField.requestFocus();
        }
        else if (e.getSource() == btnBack)
        {
            goBack();
        }

    }
}
Improve this question
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Usually we raccomend waiting between asking questions, so you can apply improvements given in one answer to the other question. \$\endgroup\$
    – Caridorc
    Jul 19, 2015 at 20:16
  • 4
    \$\begingroup\$ Good call! I'll keep that in mind. Thank you for pointing me in the right direction on how all this works. \$\endgroup\$
    – Maxim
    Jul 19, 2015 at 20:18

4 Answers 4

Reset to default
10
\$\begingroup\$

At a glance,

You should use an array for the buttons.

JButton[] buttons = new JButton[10];

Then loop to instantiate. While we're looping, we can add some lambda expressions to define what we want the buttons to do.

for (int i = 0; i < buttons.length; i++) {
  String val = Integer.toString(i)
  buttons[i] = new JButton(val);
  buttons[i].addActionListener(e -> textField.setText(val));
}

You should definitely follow Phrancis' advice on conventions, it'll make your code easier to read.

You can simplify your main method quite a bit by using a lambda expression. 

public static void main(String[] args) {
  SwingUtilities.invokeLater(Calculator::new);
}

Points to improve:

  1. You should add some consideration to dividing by 0.

  2. You can use more than one operator on the field, and it just errors out, despite being a valid operation (e.g. 3 + 2 - 1). Either disable more than 1 operation at a time or process the previous numbers when a new operator is clicked.

  3. Don't display a double value if the inputs are only integers. (e.g. 2 + 3 = 5.0)
  4. For floating point division/multiplication consider rounding the value otherwise you have endlessly repeating decimals.

These points can be resolved fairly simply, e.g. in your divide method

 if (lastCharInt != 0) {
      double result = firstCharsInt / lastCharsInt;
    } else {
     // set a message that notes that you cannot divide by 0 and clear
    }


  String stringResult = result == (int)result ? Integer.toString((int)result) : String.format(%.2f, result);

Great job with this, it's nice that you're applying your knowledge into a project, it's the best way to learn. It's better still that you've posted it here for CodeReview.

Improve this answer
\$\endgroup\$
4
  • \$\begingroup\$ Great answer, and your revised loop with addActionListener means I've deleted the comment I was writing to address the clunky actionPerformed. One edit I'd suggest to that is that the OP probably prefers their setTextTo(int) instead of textField.setText(val), since setTextTo actually appends rather than clobbers the current input. \$\endgroup\$
    – AndrewC
    Jul 19, 2015 at 23:17
  • \$\begingroup\$ Hm. Thanks for that, OP, you should make note of this. setText usually means erase contents and replace, but yours doesn't work like that. This is why it's important to follow naming conventions. \$\endgroup\$
    – Legato
    Jul 19, 2015 at 23:19
  • \$\begingroup\$ @Maxim Legato makes a good point - your setTextTo should really be called appendText or something similarly descriptive. \$\endgroup\$
    – AndrewC
    Jul 19, 2015 at 23:33
  • \$\begingroup\$ Truly amazing feedback! Using Java 8 features really makes the code easier to understand. Thank you all very much for your time, guys! \$\endgroup\$
    – Maxim
    Jul 20, 2015 at 0:55
7
\$\begingroup\$

There are some aspects of your code that make it a bit difficult in some areas, which could be cleaned up, I think. Some of it has to do with documentation, which I think is important to add throughout your code, for the next person who looks at it, which could be You in a year from now.

Indentation

Throughout your code you consistently use C-style braces and indentation, which is OK, but not ideal. The Java Code Conventions exemplify that opening braces should be placed on the same line. For example:

public static void main(String[] args) {
    EventQueue.invokeLater(new Runnable() {
        @Override
        public void run() {
            try {
                Calculator frame = new Calculator();
                frame.setVisible(true);
            }
            catch (Exception e) {
                e.printStackTrace();
            }
        }
    });
}

This list is a bit messy looking. 

private String textFieldInput;
private JButton[] myButtons =
{ btnOne, btnTwo, btnThree, btnPlus, btnFour, btnFive, btnSix, btnMinus,
    btnSeven, btnEight, btnNine, btnStar, btnPoint, btnZero, btnEquals,
    btnSlash, btnClear, btnBack };

Since all your buttons are going to be positioned, why not reflect that in your array? Just a visual aid, perhaps...

private String textFieldInput;
private JButton[] myButtons = { 
    btnOne,   btnTwo,   btnThree, btnPlus, 
    btnFour,  btnFive,  btnSix,   btnMinus,
    btnSeven, btnEight, btnNine,  btnStar, 
    btnPoint, btnZero, btnEquals, btnSlash, 
    btnClear, btnBack 
};

Your public void positionButtons() has a lot of numbers which may not mean a whole lot for someone who might not have used JButtons as much as you have. I would suggest a small amount of documentation, and a few constants (since you are repeating the width and height values a lot). For example:

public void positionButtons() {
    /**
     * The values for positioning are: setBounds(x, y, width, height)
     */
    int WIDTH = 60;
    int HEIGHT = 31;
    int BIG_WIDTH = 90;
    int BIG_HEIGHT = 48;

    // row 1
    btnOne.setBounds(46, 79, WIDTH, HEIGHT);
    btnTwo.setBounds(139, 79, WIDTH, HEIGHT);
    btnThree.setBounds(230, 79, WIDTH, HEIGHT);
    btnPlus.setBounds(340, 79, WIDTH, HEIGHT);
    // row 2
    btnFour.setBounds(46, 131, WIDTH, HEIGHT);
    btnFive.setBounds(139, 131, WIDTH, HEIGHT);
    btnSix.setBounds(230, 131, WIDTH, HEIGHT);
    btnMinus.setBounds(340, 131, WIDTH, HEIGHT);
    // row 3
    btnSeven.setBounds(46, 185, WIDTH, HEIGHT);
    btnEight.setBounds(139, 185, WIDTH, HEIGHT);
    btnNine.setBounds(230, 185, WIDTH, HEIGHT);
    btnStar.setBounds(340, 185, WIDTH, HEIGHT);
    // row 4
    btnPoint.setBounds(46, 231, WIDTH, HEIGHT);
    btnZero.setBounds(139, 231, WIDTH, HEIGHT);
    btnEquals.setBounds(230, 231, WIDTH, HEIGHT);
    btnSlash.setBounds(340, 231, WIDTH, HEIGHT);
    // row 5
    btnClear.setBounds(37, 292, BIG_WIDTH, BIG_HEIGHT);
    btnBack.setBounds(139, 292, BIG_WIDTH, BIG_HEIGHT);
}
Improve this answer
\$\endgroup\$
6
  • \$\begingroup\$ Thanks a lot for explaining your thoughts! I use auto format in eclipse so the buttons are always formatted like that each time the program runs, but a visual representation is indeed better. I keep hearing that thing about braces... I used to put the braces on the same line for a long time, but in my opinion it's more clear when being put on the next line, I guess that's just personal preference. Any thoughts about my actionPerformed method? Can that be done better? \$\endgroup\$
    – Maxim
    Jul 19, 2015 at 21:10
  • \$\begingroup\$ Braces are mostly a matter of convention, I think both of them read fine, but most Java code you'll see is indented with braces on the same line, so it's probably best to get used to that if you're going to pursue Java. I think you could use a switch statement at least to get rid of a lot of if/else statements. I'll see if I can add some example to my answer. \$\endgroup\$
    – Phrancis
    Jul 19, 2015 at 21:30
  • \$\begingroup\$ I thought about using a switch, but you can't switch on an Object right? so I can't do switch(e.getsource()) \$\endgroup\$
    – Maxim
    Jul 19, 2015 at 21:35
  • \$\begingroup\$ I'm pretty sure that you can \$\endgroup\$
    – Phrancis
    Jul 19, 2015 at 21:39
  • \$\begingroup\$ The code you wrote cannot compile and does not work since a switch can only be used on primitive types, enums and Strings? Or am I missing something? stackoverflow.com/questions/1667060/… (I compiled the code, eclipse gives me an error: "Cannot switch on a value of type Object. Only convertible int values, strings or enum variables are permitted" Anyway, thank you very much for your time, I appreciate it. \$\endgroup\$
    – Maxim
    Jul 19, 2015 at 21:44
4
\$\begingroup\$

To position your buttons you can look at GridBagLayout. It will manage the position and size of your buttons and text field for you.

There are other layout managers but this one matches your needs the closest.

Improve this answer
\$\endgroup\$
-2
\$\begingroup\$

Please lets implement the use of comments in our program. it helps other programmers understand your program better

Improve this answer
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Welcome to Code Review! While indeed comments are useful, this is a fairly old post and your answer doesn't substantially add more information. It would be good if you could expand on where you think that comments in this specific case would be necessary and why they'd help with the understanding of this specific code. \$\endgroup\$
    – ferada
    Aug 26, 2016 at 11:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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