10
\$\begingroup\$

In the spirit of April 2015 Community Challenge, I have created two Java-based calculators with the following codebase layout:

  • An interface Calculeightor for describing how values can be appended into a stream and then reduced to a single result given an operator.
  • An enum Operator to represent the operators.
  • The GUI implementation CalculeightorGUI.
  • The CLI implementation CalculeightorCLI.
  • A unit test for the CLI implementation CalculeightorCommandLineInterfaceTest.

The name uses eight as a pun, since this is written with Java 8 features in mind. Also, the unit test spells out CLI in full because appending Test to that creates another word that some may find inappropriate...

Without further ado, what follows are the code for Calculeightor and CalculeightorGUI. For the review of the other code, please look at the other review.

General feedback I am looking for:

  1. Any places where I can simplify the logic?
  2. Are Javadocs clear and concise enough?
  3. Any major faults?

Specific feedback I am looking for It's Bounty Time:

  1. I intended to keep the Calculeightor interface generic enough so that I can potentially cater for other classes extending Number, but since I have nailed down my Operator enum to support only Doubles, I have lost this flexibility. Would I be (marginally?) better off, for the purposes of this interface and the implementations, dropping the typed parameter and go with Double and Operator in it?
  2. The reason behind the private enum State is to consolidate the toggling of buttons into a series of steps that a developer can tell at a glance. As such, places that require a 'refresh of the GUI simply calls updateState(), instead of scattering the toggling logic around. Is this an OK approach, or are there alternatives to this that I should be aware of?
  3. Is there a simpler way of creating key bindings inside the GUI implementation?

Calculeightor

/**
 * A lightweight interface for describing how values can be appended into a stream and
 * then reduced to a single result given an operator.
 *
 * @param <T>
 */
public interface Calculeightor<T extends Number> {

    /**
     * @param value the value to add
     */
    void appendValue(T value);

    Stream<T> getValues();

    /**
     * @param operator the operator to use
     */
    void setOperator(BinaryOperator<T> operator);

    BinaryOperator<T> getOperator();

    /**
     * @return the result
     */
    default T getResult() {
        return getValues().reduce(getOperator()).get();
    }

    /**
     * @param value the value to display
     * @return a formatted {@link String} without trailing decimals if possible
     */
    default String display(T value) {
        return value.doubleValue() % 1 == 0 ? Integer.toString(value.intValue())
                : value.toString();
    }
}

CalculeightorGUI

/**
 * The GUI implementation of {@link Calculeightor}.
 * <p>
 * The input sequence is enforced by enabling different buttons at each stage. The
 * stages are:
 * <ol>
 * <li>Get a number</li>
 * <li>Select an operator</li>
 * <li>Get the next number</li>
 * <li>Press the {@code "="} button to calculate the answer</li>
 * </ol>
 * Results are displayed as integer values if possible.
 */
public class CalculeightorGUI extends JFrame implements Calculeightor<Double> {

    private static final long serialVersionUID = -1L;

    private final Collection<JButton> numbers = new HashSet<>();
    private final Collection<JButton> operators = new HashSet<>();
    private final JButton equalsOp = equalsButton();
    private final JLabel label = new JLabel();
    private final List<Double> inputs = new ArrayList<>(2);
    private BinaryOperator<Double> operator;
    private State state;

    /**
     * Controls what {@link Component}s need to be configured at different stages of
     * receiving input by calling {@link State#set(CalculeightorGUI)}. As such, the
     * values represent what <em>has already</em> happened, and <em>has been</em> set.
     */
    private enum State {
        INIT {
            @Override
            void set(CalculeightorGUI instance) {
                toggle(instance.operators.stream(), Stream.of(instance.equalsOp));
                instance.label.setHorizontalAlignment(SwingConstants.CENTER);
            }
        },
        FIRST_OPERAND {
            @Override
            void set(CalculeightorGUI instance) {
                toggle(instance.numbers.stream(), instance.operators.stream());
            }
        },
        OPERATOR {
            @Override
            void set(CalculeightorGUI instance) {
                toggle(instance.numbers.stream(), instance.operators.stream());
            }
        },
        SECOND_OPERAND {
            @Override
            void set(CalculeightorGUI instance) {
                toggle(instance.numbers.stream(), Stream.of(instance.equalsOp));
            }
        },
        EQUALS {
            @Override
            void set(CalculeightorGUI instance) {
                toggle(instance.numbers.stream(), Stream.of(instance.equalsOp));
                instance.inputs.clear();
            }
        };

        State next() {
            return this == EQUALS ? FIRST_OPERAND : values()[ordinal() + 1];
        }

        abstract void set(CalculeightorGUI instance);

        private static <T extends Component> void toggle(Stream<T>... components) {
            Stream.of(components).flatMap(Function.identity())
                    .forEach(c -> c.setEnabled(!c.isEnabled()));
        }
    }

    public CalculeightorGUI() {
        setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        setTitle(getClass().getSimpleName());
        getContentPane().setLayout(new GridLayout(0, 1));
        Stream.of(numbersPanel(), operatorsPanel(), panelOf(Stream.of(equalsOp, label)))
                .forEach(this::add);
        updateState();
        pack();
        setVisible(true);
    }

    /**
     * Updates the latest state and also the input configuration.
     */
    private void updateState() {
        (state = state == null ? State.INIT : state.next()).set(this);
    }

    private JPanel numbersPanel() {
        return panelOf(IntStream.rangeClosed(0, 9).boxed(), numbers, event -> {
            Double value = Double.valueOf((((JButton) event.getSource()).getText()));
            appendValue(value);
            label.setText(Integer.toString(value.intValue()));
            updateState();
        });
    }

    private JPanel operatorsPanel() {
        return panelOf(Stream.of(Operator.values()), operators, event -> {
            String value = ((JButton) event.getSource()).getText();
            setOperator(Operator.of(value));
            label.setText(value);
            updateState();
        });
    }

    /**
     * Creates {@link JButton}s representing each stream's elements.
     *
     * @param values
     * @param collection the collection to add the created {@link JButton}s to
     * @param listener the listener to receive events from the {@link JButton}s
     * @return a single-row {@link JPanel} containing the created {@link JButton}s
     */
    private <T> JPanel panelOf(Stream<T> values, Collection<JButton> collection,
            ActionListener listener) {
        return panelOf(values.map(v -> createButton(v, listener))
                .peek(collection::add));
    }

    /**
     * @param components
     * @return a single-row {@link JPanel} containing the {@link Component}s
     */
    private static <T extends Component> JPanel panelOf(Stream<T> components) {
        JPanel panel = new JPanel(new GridLayout());
        components.forEach(panel::add);
        return panel;
    }

    private JButton equalsButton() {
        return createButton("=", event -> {
            Double value = getResult();
            label.setText(display(value));
            label.setForeground(value.isInfinite() ? Color.RED : Color.BLACK);
            updateState();
        });
    }

    /**
     * @param input the input to use
     * @param listener the listener to receive events from the {@link JButton}
     * @return a {@link JButton} with the input as its text and a key binding of the
     *         first character while the window is in focus
     */
    private static JButton createButton(Object input, ActionListener listener) {
        String text = input.toString();
        JButton button = new JButton(text);
        button.addActionListener(listener);
        button.getInputMap(JComponent.WHEN_IN_FOCUSED_WINDOW).put(
                KeyStroke.getKeyStroke(text.charAt(0)), text);
        button.getActionMap().put(text, getAction(button));
        return button;
    }

    /**
     * @param button the {@link JButton} to configure for
     * @return an {@link Action} that clicks the button
     */
    @SuppressWarnings("serial")
    private static Action getAction(JButton button) {
        return new AbstractAction() {
            @Override
            public void actionPerformed(ActionEvent e) {
                button.doClick();
            }
        };
    }

    @Override
    public void appendValue(Double value) {
        inputs.add(value);
    }

    @Override
    public Stream<Double> getValues() {
        return inputs.stream();
    }

    @Override
    public void setOperator(BinaryOperator<Double> operator) {
        this.operator = operator;
    }

    @Override
    public BinaryOperator<Double> getOperator() {
        return operator;
    }

    public static void main(String[] args) {
        new CalculeightorGUI();
    }
}
\$\endgroup\$
5
+100
\$\begingroup\$

For starters, my main() method should use SwingUtilities.invokeLater():

public static void main(String[] args) {
    SwingUtilities.invokeLater(() -> { new CalculeightorGUI(); });
}

I should also use my State enum to handle the display of inputs, for example I can have the following two static helper methods in the enum:

private static void display(CalculeightorGUI instance, Double value) {
    instance.label.setText(Integer.valueOf(value.intValue()).toString());
}

private static void display(CalculeightorGUI instance, String text) {
    instance.label.setText(text);
}

And the code for FIRST_OPERAND, OPERATOR and SECOND_OPERAND can be modified as such:

FIRST_OPERAND {
    @Override
    void set(CalculeightorGUI instance) {
        toggle(instance.numbers.stream(), instance.operators.stream());
        List<Double> inputs = instance.inputs;
        display(instance, inputs.get(inputs.size() - 1));
    }
},
OPERATOR {
    @Override
    void set(CalculeightorGUI instance) {
        toggle(instance.numbers.stream(), instance.operators.stream());
        display(instance, instance.operator.toString());
    }
},
SECOND_OPERAND {
    @Override
    void set(CalculeightorGUI instance) {
        toggle(instance.numbers.stream(), Stream.of(instance.equals));
        List<Double> inputs = instance.inputs;
        display(instance, inputs.get(inputs.size() - 1));
    }
}

This simplifies (slightly) my ActionListeners for the numbers and operators buttons:

private JPanel numbersPanel() {
    return panelOf(IntStream.rangeClosed(0, 9).boxed(), numbers, event -> {
        appendValue(Double.valueOf((((JButton) event.getSource()).getText())));
        updateState();
    });
}

private JPanel operatorsPanel() {
    return panelOf(Stream.of(Operator.values()), operators, event -> {
        setOperator(Operator.of(((JButton) event.getSource()).getText()));
        updateState();
    });
}

edit

Actually, why stop at the operands and operator when I can do the same thing for the equals button too?

I figured I can also create a Double result field in my class:

private final List<Double> inputs = new ArrayList<>(2);
private Double result = null; // to store the result
private State state = null;
private Operator operator;

Then my equalsButton() can be simplified as such:

private JButton equalsButton() {
    JButton button = createButton("=", event -> {
        result = inputs.stream().reduce(operator).get();
    });
    return button;
}

The Operator.EQUALS implementation then becomes:

EQUALS {
    @Override
    void set(CalculeightorGUI instance) {
        toggle(instance.numbers.stream(), Stream.of(instance.equals));
        instance.label.setText(instance.display(instance.result));
        instance.label.setForeground(
                instance.result.isInfinite() ? Color.RED : Color.BLACK);
        instance.inputs.clear();
    }
}

The equalsButton() changes above does not call the updateState(), ditto for the ActionListeners specified in numbersPanel() and operatorsPanel(), because I can move that to my createButton() code to 'compose' invoking the listener argument with calling updateState() too. This means I have to un-static the method createButton() itself.

private JButton createButton(Object input, ActionListener listener) {
    String text = input.toString();
    JButton button = new JButton(text);
    button.addActionListener(event -> {
        listener.actionPerformed(event); updateState(); });
    button.getInputMap(JComponent.WHEN_IN_FOCUSED_WINDOW).put(
            KeyStroke.getKeyStroke(text.charAt(0)), text);
    button.getActionMap().put(text, getAction(button));
    return button;
}
\$\endgroup\$
4
\$\begingroup\$
> public static void main(String[] args) {
>     SwingUtilities.invokeLater(() -> { new CalculeightorGUI(); }); }

The content of your main can actually be just SwingUtilities.invokeLater(CalculeightorGUI::new);

\$\endgroup\$
  • 1
    \$\begingroup\$ +1 on your answer, will leave my own answer as the original so as not to diminish your input here. :D \$\endgroup\$ – h.j.k. Apr 28 '15 at 1:01
  • 1
    \$\begingroup\$ Thanks, I really enjoy both your question and answer! Makes for a good study. \$\endgroup\$ – Legato Apr 28 '15 at 1:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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