Integer sum program

Question

This is a little assignment I am working on. I am a beginner Java programmer and would love some advice on how to improve this code.

For more context, here are the assignment details:

Create a complete program that has the ability to store and display integer values in an array. The maximum number of values that your program should be able to handle is 10.

• Add additional elements to the array using screen input (textbox and button).
• Remove array elements based on screen input (textbox and button).
• List all of the elements in the array and compute the sum of all the elements.
• List the even elements in the array and compute the sum of the even elements.
• List the odd elements in the array and compute the sum of the odd elements.

    import javax.swing.*;
    import java.awt.*;
    import java.awt.event.ActionEvent;
    import java.awt.event.ActionListener;
    import java.util.Arrays;

    public class sumElement implements ActionListener {

    public static JButton add, remove, sumAll, sumEven, sumOdd;
    public static JTextField inputField;
    public static JTextArea textArea;
    public static int counter = 0;
    public static int[] numbers = new int[10];
    public static JLabel titleText;

    public static void main(String[] args) {
        // Frame
        JFrame frame = new JFrame("Integer Sums");
        frame.setSize(178, 240);
        frame.setResizable(false);
        frame.setLocationRelativeTo(null);
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);

        // Container panel
        JPanel container = new JPanel();
        container.setLayout(new BoxLayout(container, BoxLayout.PAGE_AXIS));
        frame.setContentPane(container);

        // Title panel
        JPanel titlePane = new JPanel();
        titleText = new JLabel("Integer Sums");

        // Content panel
        JPanel content = new JPanel();
        content.setPreferredSize(new Dimension(300, 180));
        content.setLayout(null);

        // Buttons
        add = new JButton("Add");
        add.setBounds(4, 50, 80, 20);
        add.addActionListener(new sumElement());

        remove = new JButton("Remove");
        remove.setBounds(88, 50, 80, 20);
        remove.addActionListener(new sumElement());

        sumAll = new JButton("Sum All");
        sumAll.setBounds(4, 74, 164, 20);
        sumAll.addActionListener(new sumElement());

        sumEven = new JButton("Sum Even");
        sumEven.setBounds(4, 98, 164, 20);
        sumEven.addActionListener(new sumElement());

        sumOdd = new JButton("Sum Odd");
        sumOdd.setBounds(4, 122, 164, 20);
        sumOdd.addActionListener(new sumElement());

        // Text area, and text field
        inputField = new JTextField();
        inputField.setBounds(4, 21, 164, 25);

        JLabel inputLabel = new JLabel("Input integer value below:");
        inputLabel.setBounds(12, 6, 164, 13);

        textArea = new JTextArea();
        JScrollPane scrollPane = new JScrollPane(textArea);
        scrollPane.setBounds(4, 144, 165, 36);

        // Adding everything
        container.add(titlePane);
        titlePane.add(titleText);
        container.add(content);
        content.add(inputLabel);
        content.add(inputField);
        content.add(scrollPane);
        content.add(add);
        content.add(remove);
        content.add(sumAll);
        content.add(sumEven);
        content.add(sumOdd);

        // Extras
        frame.toFront();
        frame.setVisible(true);
    }

    public void actionPerformed(ActionEvent event) {
        if (event.getActionCommand().equals("Add")) {
            if (counter == 10) {
                titleText.setText("Error: too many values");
            } else {
                numbers[counter] = Integer.parseInt(inputField.getText());
                textArea.setText(Arrays.toString(numbers));
                counter++;
            }
        } else if (event.getActionCommand().equals("Remove")) {
            for (int i = 0; i <= counter; i++) {
                if (Integer.parseInt(inputField.getText()) == numbers[i]) {
                    for (int x = i; x <= counter; x++) {
                        numbers[x] = numbers[x + 1];
                    }
                }
            }
            counter--;
            textArea.setText(Arrays.toString(numbers));
        } else if (event.getActionCommand().equals("Sum All")) {
            int sum = 0;
            for (int i = 0; i <= counter; i++) {
                sum += numbers[i];
            }
            titleText.setText("Sum is " + sum);
        } else if (event.getActionCommand().equals("Sum Even")) {
            int sum = 0;
            for (int i = 0; i <= counter; i++) {
                if (numbers[i] % 2 == 0) {
                    sum += numbers[i];
                }
            }
            titleText.setText("Sum of even values is: " + sum);
        } else if (event.getActionCommand().equals("Sum Odd")) {
            int sum = 0;
            for (int i = 0; i <= counter; i++) {
                if (numbers[i] % 2 != 0) {
                    sum += numbers[i];
                }
            }
            titleText.setText("Sum of odd values is: " + sum);
        }
    }
}

Answer 1

Avoid relying on display for business logic

        if (event.getActionCommand().equals("Add")) {

Be careful with this kind of thing. If you write a real application, it's not uncommon to have to support multiple languages. In some places, it may be required. E.g. French in Quebec. If you are displaying the French word for Add, then it probably won't match the English word that you are using here.

Even without multiple languages, consider what happens if you change a label. E.g. in this case, you might switch to the more precise Append. You'd have to make that change in multiple places.

Note: by business logic I mean the require logic for the application. I.e. what is the "business" of the application. In this case, determining what to do in response to the input. By display, I mean that "Add" is text that you are displaying. There are often times when you want to change the display without changing the logic of the application. Or vice versa.

I am more familiar with the pattern where each event gets a different ActionListener rather than passing the same one and switching inside. That would eliminate the if/else structure, but it might make the variable management more difficult (e.g. counter and numbers).

Java 7 added the ability to use a switch on a String. This is often more readable than an if/else ladder.

Ranges are more robust than endpoints

            if (counter == 10) {

Consider

            if (counter >= 10) {

The same effect if counter is 10, but this also handles values greater than 10 as well. Consider the possibility of adding multiple values at once. If counter goes to 11, then the original code would keep failing. This version would stop.

There are some circumstances when ranges won't work, but this doesn't seem like one of them.

Bugs?

            for (int i = 0; i <= counter; i++) {
                if (Integer.parseInt(inputField.getText()) == numbers[i]) {
                    for (int x = i; x <= counter; x++) {
                        numbers[x] = numbers[x + 1];
                    }
                }
            }
            counter--;

First, there's no need to parse the value multiple times. We can do that before the loop. But that's not a bug, just a point of style or efficiency.

This will go past the end of the array. It should be x < counter, not x <= counter. As the add method is written, counter is the used size of the array, not the last element.

And the inner loop should be x < counter - 1 or similar. Or update counter before the inner for loop, but that would cause other changes at the same time. Otherwise, x + 1 will equal counter, which as we just saw, is outside the used portion of the array.

What if the same value appears multiple times in the array? This would remove it from the array multiple times but only update counter once.

A couple possible solutions:

            int needle = Integer.parseInt(inputField.getText());

            for (int i = 0; i < counter; i++) {
                if (needle == numbers[i]) {
                    counter--;

                    for (int x = i; x < counter; x++) {
                        numbers[x] = numbers[x + 1];
                    }
                }
            }

This updates counter each time it removes an element. It can remove multiple elements in one call. This way we can move the update to counter before the loop where we move the elements of the array.

Or

            int needle = Integer.parseInt(inputField.getText());

            int i = 0;
            for (; i < counter && needle != numbers[i]; i++) ;

            if (i < counter) {
                counter--;

                for (; i < counter; i++) {
                    numbers[i] = numbers[i + 1];
                }
            }

This only removes the first match.

Consider using a List instead of an array

Or if you change numbers to a List rather than an array, you could do away with counter altogether.

            numbers.remove(Integer.parseInt(inputField.getText()));

This would replace the last option above, removing just the first match. Or

            Integer needle = Integer.parseInt(inputField.getText());
            while (numbers.remove(needle)) ;

To remove all occurrences. Or more fancily but briefly

            numbers.removeAll(Arrays.asList(Integer.parseInt(inputField.getText())));

A List would also allow us to change things like

            for (int i = 0; i <= counter; i++) {
                sum += numbers[i];
            }

to something like

            for (int number : numbers) {
                sum += number;
            }

Which would eliminate the bug related to counter as well (should be less than counter not less than or equal here too).