current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 2 down vote favorite

I'm practicing Swing effects. Everything works but for some reason I get a ton of error messages in the command prompt when the fields are empty, despite my attempt to add some exception handling (this has made me realize I need to practice that a lot more).

    /* Author: Luigi Vincent
Practicing Java Swing button and button events; Window that does simple calculation on two numbers
TO DO: Figure out why command prompt show so many messages when fields are empty & stop it
*/

import javax.swing.*;
import java.awt.*;
import java.awt.event.*;
import java.text.DecimalFormat;


public class SimpleCalc extends JFrame {

    SimpleCalc (String name) {
        super(name);

        setLayout(new FlowLayout());

        // Fields for input and output
        JTextField result = new JTextField(10);
        JTextField num1 = new JTextField(5);
        JTextField num2 = new JTextField(5);
        result.setEditable(false);

        // Buttons for operations
        JButton b_add = new JButton("Add");
        JButton b_sub = new JButton("Subtract");
        JButton b_mul = new JButton("Multiply");
        JButton b_div = new JButton("Divide");

        // Listener to handle operations
        ActionListener calculator = new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent e) {
                try {
                    if (num1.getText().length() == 0 || num2.getText().length() == 0) {
                        throw new Exception();
                    }
                }
                catch (Exception ex){
                    JOptionPane.showMessageDialog(null, "Field missing", "Warning", JOptionPane.WARNING_MESSAGE);
                }

                Double alpha = Double.parseDouble(num1.getText());
                Double beta = Double.parseDouble(num2.getText());
                Double omega = null;
                DecimalFormat f = new DecimalFormat("##.00");

                if (e.getSource() == b_add) {
                    omega = alpha + beta;
                }
                else if (e.getSource() == b_sub) {
                    omega = alpha - beta;
                }
                else if (e.getSource() == b_mul) {
                    omega = alpha * beta;
                }
                else if (e.getSource() == b_div) {
                    omega = alpha / beta;
                }
                result.setText(f.format(omega).toString());
            }
        };
       // Register buttons to event listener
        b_add.addActionListener(calculator);
        b_sub.addActionListener(calculator);
        b_mul.addActionListener(calculator);
        b_div.addActionListener(calculator);

       // Add everything to frame + labels    
        add(new JLabel("First Number"));
        add(num1);
        add(new JLabel("Second Number"));
        add(num2);
        add(new JLabel("Result"));
        add(result);
        add(b_add);
        add(b_sub);
        add(b_mul);
        add(b_div);
    }
    public static void main(String[] args) {
        SimpleCalc frame = new SimpleCalc("Simple Calculator");
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setSize(500, 100);
        frame.setLocationRelativeTo(null);
        frame.setVisible(true);
    }
}

I'm open to any and all improvements possible.

share|improve this question
    
Sorry the missing 'new' was an editing error, I renamed the class before posting the question, accidentally removing it. I added it and double checked, The code compiles and works fine, I just want to address the errors in the command prompt when the field is empty/how to prevent that and otherwise any other tips regarding implementation, usability and legibility. –  Legato Jul 14 at 5:35

1 Answer 1

up vote 3 down vote accepted

The major problem I can see is this piece of code : 

            try {
                if (num1.getText().length() == 0 || num2.getText().length() == 0) {
                    throw new Exception();
                }
            }
            catch (Exception ex){
                JOptionPane.showMessageDialog(null, "Field missing", "Warning", JOptionPane.WARNING_MESSAGE);
            }

If you throw inside a try-catch, you will not exit the function. Since your condition is num1.getText().length() == 0 when you try to do Double.parseDouble(num1.getTest() well your string is empty so this throw an exception.

You should not use throw and catch to manage an error, not in this way. You should first remove the try-catch and work with a simple if :

if (num1.getText().isEmpty() || num2.getText().isEmpty()) {
    JOptionPane.showMessageDialog(null, "Field missing", "Warning", JOptionPane.WARNING_MESSAGE);
    throw new Exception();
}

I would not suggested that since now you add a complexity to your code.

  • The method now need to declare throws Exception (which is not possible for actionPerformed()), now the caller need to manage this possible outcome.

  • Exception would be too generic.

  • I guess that this case would happen a lot,so throwing an Exception could become heavy for the program.

The real solution is to only return from the method. You've already warned the user that he need to do something (you could be more clear) and you don't want to execute the rest of method.

There is some general notion that are lacking in your code, but I won't cover it all, since I mention it in others answer :

  • There is naming convetions in Java, b_add does not follow it, it should be bAdd (but it could simply be add)

  • You should not extend JFrame, but rather just have a private JFrame, you're not adding any functionnality to JFrame you're only using it. (Composition over inheritance) answer that cover a bit that part

share|improve this answer
    
Mind at least tell me which of those 'other answers' I should look at? Thank you for the input, it was very helpful! I think I was forcing myself to use an Exception when it wasn't necessary, and the parse.double completely eluded me. Quick question about composition over inheritance, if I did it the plain way wouldn't all of this have to be in the main method? (I read somewhere around here that that was bad practice...) –  Legato Jul 15 at 2:23
    
No, not at all. You will have a field private JFrame mainFrame; and your constructor will still take a String. You will then make a new JFrame(name) and change all your setLayout and add (in fact all the JFrame call) like this mainFrame.setLayout(""). For another answer to look read this one about generic style : codereview.stackexchange.com/a/55904 –  Marc-Andre Jul 15 at 2:31
    
Did you need more explanation about he double part or you were good ? And I didn't linked specific answers to not "advertise" myself too much! There are plenty of good answers here that will help you about the generic stuff of Jave! I would recommend you to browse the java and swing tag for more information (reading code can help a lot!) –  Marc-Andre Jul 15 at 2:33

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.