7
\$\begingroup\$

I was searching for a way to exercise the Java 8 concepts I've been learning, especially pertaining to Lambdas. Incidentally, I hadn't used any swing in a while and wanted a refresher. This is the result of these two pursuits. 

import java.awt.GridLayout;
import javax.swing.JComboBox;
import javax.swing.JFrame;
import javax.swing.JOptionPane;
import javax.swing.JPanel;
import javax.swing.JTextField;
import javax.swing.UIManager;
import javax.swing.SwingUtilities;
import javax.swing.WindowConstants;

public class EquationCalculator {
    static interface Equation {
        double compute(double val1, double val2);
    }

    public static void main(String[] args) {
        try {
            for (UIManager.LookAndFeelInfo info : UIManager.getInstalledLookAndFeels()) {
                if ("Nimbus".equals(info.getName())) {
                    UIManager.setLookAndFeel(info.getClassName());
                    break;
                }
            }
        } catch (Exception e) {
            e.printStackTrace();
        }

        SwingUtilities.invokeLater(() -> new EquationCalculator());
    }

    public EquationCalculator() {
        JFrame frame = new JFrame("Basic Operations");
        JPanel panel = new JPanel(new GridLayout(1, 3));
        JTextField first = new JTextField("2");
        JTextField second = new JTextField("3");
        JComboBox<String> calculate = new JComboBox<>();

        calculate.addItem("Sum");
        calculate.addItem("Difference");
        calculate.addItem("Product");
        calculate.addItem("Quotient");
        calculate.addItem("Exponent");
        calculate.addActionListener(
            e -> JOptionPane.showMessageDialog(
                null, "The result is " +
                calculate(
                    String.valueOf(calculate.getSelectedItem()),
                    Double.parseDouble(first.getText()),
                    Double.parseDouble(second.getText())
                ), 
                "Result", JOptionPane.INFORMATION_MESSAGE
            )
        );

        panel.add(first);
        panel.add(second);
        panel.add(calculate);

        frame.add(panel);
        frame.pack();
        frame.setLocationRelativeTo(null);
        frame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
        frame.setVisible(true);
    }

    public static double calculate(String operation, double val1, double val2) {
        Equation add = (alpha, beta) -> alpha + beta;
        Equation subtract = (alpha, beta) -> alpha - beta;
        Equation multiply = (alpha, beta) -> alpha * beta;
        Equation divide = (alpha, beta) -> alpha / beta;
        Equation exponentiate = (alpha, beta) -> Math.pow(alpha, beta);

        switch(operation) {
            case "Sum":
                return add.compute(val1, val2);
            case "Difference":
                return subtract.compute(val1, val2);
            case "Product":
                return multiply.compute(val1, val2);
            case "Exponent":
                return exponentiate.compute(val1, val2);
        }
        return divide.compute(val1, val2);
    }
}

I recognize it's simple, and I can imagine there's likely a bit of "learned how to use a hammer...everything looks like a nail" going on, but I'd nonetheless appreciate any pinpointing good design choices vs poor ones and where I could improve.

\$\endgroup\$

2 Answers 2

Reset to default
8
\$\begingroup\$

Don't use String labels of GUI elements in flow control

It's not good to use the String labels of GUI elements to make decisions. It would be slightly better if you made "Sum", "Difference", ... global constants, but only slightly.

An enum would be better for this purpose:

enum Operation {
    Sum("Sum", (x, y) -> x + y),
    Difference("Difference", (x, y) -> x - y),
    Product("Product", (x, y) -> x * y),
    Quotient("Quotient", (x, y) -> x / y),
    Exponent("Exponent", Math::pow)
    ;

    private final String label;
    private final Equation equation;

    private Operation(String label, Equation equation) {
        this.label = label;
        this.equation = equation;
    }

    @Override
    public String toString() {
        return label;
    }
}

With this, you could create the JComboBox with Operation items instead of String:

    JComboBox<Operation> calculate = new JComboBox<>();

    calculate.addItem(Operation.Sum);
    calculate.addItem(Operation.Difference);
    calculate.addItem(Operation.Product);
    calculate.addItem(Operation.Quotient);
    calculate.addItem(Operation.Exponent);

Change the call to calculate to this:

    calculate(
            ((Operation) calculate.getSelectedItem()).equation,
            Double.parseDouble(first.getText()),
            Double.parseDouble(second.getText())
    ),

And simplify calculate itself to this:

public static double calculate(Equation operation, double val1, double val2) {
    return operation.compute(val1, val2);
}

Avoid pointless evaluations

In calculate, you declare all the equation types at the top but only use one of them. This is pointless and wasteful.

Simplify lambdas with method references

These lambdas can be replaced with method references:

    SwingUtilities.invokeLater(() -> new EquationCalculator());
    // ...
    Equation exponentiate = (alpha, beta) -> Math.pow(alpha, beta);

Like this:

    SwingUtilities.invokeLater(EquationCalculator::new);
    // ...
    Equation exponentiate = Math::pow;

Pointless to print stack trace in main

If an exception is thrown in main, a stack trace will be printed on the console. There's no need to catch an exception and do the same manually.

Also beware, it's not a good practice to catch Exception. It's recommended to catch specific exceptions, both to signal to readers the things that can possibly go wrong, and also to avoid truly unexpected exceptions getting caught by the blanket catch.

\$\endgroup\$
2
  • 2
    \$\begingroup\$ Exactly what I was looking for. \$\endgroup\$ Commented Mar 1, 2015 at 7:09
  • \$\begingroup\$ Awesome, glad I could help! \$\endgroup\$ Commented Mar 1, 2015 at 7:13
5
\$\begingroup\$

I just have three small points:

  • default: if you have a switch, it's better to put in a default return value instead of returning the default right after the switch. Also, having an expected value as the default is a bit confusing. I would add the divide case, and then throw an exception for the default (eg IllegalArgumentException).
  • The Java8 functionality in calculate doesn't add anything except complexity and overhead (why not directly return eg val1 + val2?), but in the other two places you used it well.
  • usability: having the action listener on the combo box is a bit hard to use (also, it's not very intuitive), generally, I would add an extra button.

One of the advantages of lambdas is that you can now pass functions without having to define an interface yourself, and that you can pass implementations with little writing overhead. For example, your code could look like this:

public static double calculate(String operation, double val1, double val2) {
    switch(operation) {
        case "Sum":
            return calculate((x,y) -> x + y, val1, val2);
        //[...]

    }
}

public static double calculate(Equation equation, double val1, double val2) {
    return equation.compute(val1, val2);
}

Of course, this still doesn't add anything over the much simpler implementation of just returning val1 + val2 directly, but it should show the advantages of lambdas in general.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.