7
\$\begingroup\$

I've been learning JavaFX and wanted to recreate Simple calculator implemented with lambdas. It's also my first time using CSS, whose capability has convinced me of the wonders and appeal of JavaFX over swing.

Hitherto, I've only somewhat used swing and jumping into JavaFX this program is now my most complex, I like how it came out and would really appreciate a general review on anything I may be doing wrong/inefficiently/unconventionally.

The ultimate goal here would be of course to have it be a standalone jar.

example run of the program

Main class:

import javafx.application.Application;
import javafx.scene.Group;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.control.ComboBox;
import javafx.scene.control.Label;
import javafx.scene.control.TextField;
import javafx.scene.layout.HBox;
import javafx.scene.paint.Color;
import javafx.stage.Stage;
import javafx.stage.StageStyle;


public class Calculator extends Application {
    public static void main(String[] args) {
        launch(args);
    }

    @Override
    public void start(Stage stage) {
        stage.setTitle("Thank you CodeReview");

        ComboBox<Operation> operations = new ComboBox<>();
        for (Operation op : Operation.values()) {
            operations.getItems().add(op);
        }
        operations.setValue(Operation.values()[0]);

        TextField[] input = new TextField[2];
        for (int i = 0; i < input.length; i++) {
            input[i] = new TextField();
            input[i].setPromptText("Enter a number");
        }

        Label result = new Label("Click calculate for result");

        Button calculate = new Button("Calculate");
        calculate.setOnAction(
            e -> result.setText(
                computeResult(
                    (Operation)operations.getValue(),
                    input[0].getText(),
                    input[1].getText()
                )
            )
        );

        HBox box = new HBox();
        box.getChildren().addAll(
            input[0], operations, input[1], calculate, result
        );

        Group root = new Group();
        root.getChildren().add(box);

        Scene scene = new Scene(root);
        scene.getStylesheets().add("Calculator.css");
        scene.setFill(Color.WHITE);

        stage.initStyle(StageStyle.UTILITY);
        stage.setScene(scene);
        stage.sizeToScene();
        stage.centerOnScreen();
        stage.show();
    }

    private static String computeResult(Operation op, String val1, String val2) {
        double result = op.getEquation().compute(
            Double.parseDouble(val1),
            Double.parseDouble(val2)
        );

        return result == (int)result ? 
            String.valueOf((int)result) :
            String.format("%.2f", result)
        ;
    }
}

Equation interface:

interface Equation {
    double compute(double val1, double val2);
}

Available operations:

enum Operation {
    ADD((x, y) -> x + y),
    SUBTRACT((x, y) -> x - y),
    MULTIPLY((x, y) -> x * y),
    DIVIDE((x, y) -> x / y),
    MOD((x, y) -> x % y),
    HYPOTENUSE(Math::hypot),
    EXPONENTIATE(Math::pow);

    private final Equation equation;

    Operation(Equation equation) {
        this.equation = equation;
    }

    public Equation getEquation() {
        return this.equation;
    }

    public String toString() {
        return this.name().charAt(0) + this.name().substring(1).toLowerCase();
    }
}

Applied CSS:

* {
    calculator-base: linear-gradient(#2A5058, #61a2b1);
}

.root {
    -fx-font-family: "Georgia";
}

.button {
    -fx-font-weight: bold;
    -fx-text-fill: calculator-base
}
.button:hover {
    -fx-text-fill: white;
    -fx-background-color: calculator-base;
}
.button:pressed {
    -fx-text-fill: white;
    -fx-font-family: "Courier";
    -fx-background-insets: 0,0 0 5 0, 0 0 6 0, 0 0 7 0;
    -fx-background-radius: 8;
    -fx-background-color: 
        linear-gradient(from 0% 93% to 0% 100%, #2A5058 0%, #61a2b1 100%),
        #2A5058,
        #61a2b1,
        radial-gradient(center 50% 50%, radius 100%, #61a2b1, #2A5058);
    -fx-effect: dropshadow(gaussian , rgba(0,0,0,0.75) , 4,0,0,1);
    -fx-font-size: 1.1em;
}

.combo-box-base .arrow {
    -fx-background-color: white;
}

.combo-box-base .arrow-button {
    -fx-background-color: calculator-base;
}

.combo-box .cell {
    -fx-text-fill: calculator-base;
}
.combo-box .cell:hover {
    -fx-font-weight: bold;
    -fx-text-fill: white;
    -fx-background-color: calculator-base;
}
.combo-box .cell:focused {
    -fx-font-weight: bold;
    -fx-text-fill: white;
    -fx-background-color: calculator-base;
}

.label {
    -fx-font-weight: bold;
    -fx-text-fill: calculator-base;
    -fx-padding: 4;
}

.text-field {
    -fx-text-fill: calculator-base;
}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Re the CSS, it seems it's possible to at least define color constants? \$\endgroup\$
    – ferada
    Mar 7, 2015 at 22:35
  • \$\begingroup\$ @ferada Thank you for that. Useful tip I'll definitely be employing in the future. \$\endgroup\$
    – Legato
    Mar 7, 2015 at 22:37
  • 1
    \$\begingroup\$ +1 for title (I have nothing else to add other than why aren't you using a css preprocessor like stylus?) \$\endgroup\$
    – Dan
    Mar 8, 2015 at 0:26
  • 2
    \$\begingroup\$ That's the first I'm hearing about such a thing. \$\endgroup\$
    – Legato
    Mar 8, 2015 at 0:30

1 Answer 1

3
\$\begingroup\$

Summary

  • Lambda in enum
  • Maybe use java.util.function.BinaryOperator
  • Law of Demeter
  • Shorter methods
  • Extensibility
  • CSS
  • Collections addAll()

Lambda in enum

Now this is really cool stuff. I love reading the Operation enum over and over again. It is beautiful.

java.util.function.BinaryOperator

Your definition of interface Equation seems redundant with BinaryOperator<Double>. Maybe you want to use BinaryOperator<Double> instead of your own interface Equation.

Law of Demeter

Your enum Operation forces the caller (in this case it's computeResult()) to violate the Law of Demeter.

You could instead:

public enum Operation implements Equation {
    ADD((x, y) -> x + y),
    // ...
    private final Equation equation;
    // ...
    public double compute(final double val1, final double val2) {
        return equation.compute(val1, val2);
    }
}

Then a call to getEquation() would no longer be required, the Operation could be used directly because it's an Equation itself. (Or BinaryOperator<Double> if you go for that.)

Shorter methods

start() is too long, I'd split it. You will have to move some of the variables into fields for that, and I think that's okay. It also comes at another benefit, you could use a method reference instead of a lambda for calculate.setOnAction() then.

Extensibility

The disadvantage of an enum is that you cannot add new values at runtime. With that respect, an enum is somewhat a switch-case in a very smart disguise. It comes with all the OO stuff and polymorphism, but when you want new elements, you still have to change the original source. But it's easy to break that. You could use a Set<Equation> which you initialize with addAll(Operation.values()) (requires that Operation implements Equation which is recommended for the Law of Demeter anyway).

Not sure whether you really want to make that a requirement for your app, I just thought it might be interesting to discuss.

CSS

In case two selectors have intentionally the same properties, you could make that more obvious by joining their declarations. For example, you could join

.combo-box .cell:hover {
    -fx-font-weight: bold;
    -fx-text-fill: white;
    -fx-background-color: calculator-base;
}
.combo-box .cell:focused {
    -fx-font-weight: bold;
    -fx-text-fill: white;
    -fx-background-color: calculator-base;
}

into

.combo-box .cell:hover, .combo-box .cell:focused {
    -fx-font-weight: bold;
    -fx-text-fill: white;
    -fx-background-color: calculator-base;
}

You can also have multiple rules for the same selector. With CSS you're always in a dilemma: Do you go for selector-ordering or for property-ordering... I tend to follow a mixed approach, trying to avoid redundancy as much as possible.

If your CSS grows bigger and you tend to use the same property values in different places, you might want to go for a CSS preprocessor in order to replace those values with preprocessor constants. This can significantly increase the maintainability of non-trivial CSS.

Collections addAll()

I think your code

ComboBox<Operation> operations = new ComboBox<>();
for (Operation op : Operation.values()) {
    operations.getItems().add(op);
}

could be simplified to

ComboBox<Operation> operations = new ComboBox<>();
operations.getItems().addAll(EnumSet.allOf(Operation.class));

If the API can do something for me, I tend to delegate things to the API.

\$\endgroup\$
1
  • \$\begingroup\$ Oh! I wasn't aware of BinaryOperator. This is quite a helpful answer, thank you. \$\endgroup\$
    – Legato
    Mar 8, 2015 at 14:53

Your Answer

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

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