5
\$\begingroup\$

I made a simple JavaFX calculator. It does basic calculations, and works to the best of my knowledge. However, I'm a novice at both Java and JavaFX, so I seriously doubt this is as efficient and clean as possible. I personally think it could be more modular, maybe put those huge loops in functions or something.

package calculator;

import javafx.application.Application;

import javafx.scene.Scene;
import javafx.scene.Group;

import javafx.scene.control.Label;
import javafx.scene.control.Button;

import javafx.scene.layout.BorderPane;
import javafx.scene.layout.GridPane;
import javafx.scene.layout.VBox;
import javafx.scene.layout.RowConstraints;
import javafx.scene.layout.ColumnConstraints;

import javafx.scene.paint.Color;
import javafx.event.ActionEvent;
import javafx.event.EventHandler;

import javafx.stage.Stage;

import javafx.geometry.HPos;

import java.util.ArrayList;

import javax.script.ScriptEngineManager;
import javax.script.ScriptEngine;
import javax.script.ScriptException;

public class Calculator extends Application{
    private boolean operatorAlreadyPressed = false;
    private boolean secondOperand = false;
    private boolean scriptExceptionOccurred = false;
    public static void main(String[] args){
            Application.launch(args);
    }
    public void start(Stage primaryStage) throws ScriptException{
        int windowWidth = 190;
        int windowHeight = 250;
        primaryStage.setTitle("Calculator");
        primaryStage.setWidth(windowWidth);
        primaryStage.setHeight(windowHeight);
//      primaryStage.setResizable(false);
        VBox root = new VBox();
        Scene scene = new Scene(root, windowWidth, windowHeight, Color.WHITE);

        Label expressionLabel = new Label();
//      expressionLabel.setEditable(false);

        GridPane numberGrid = new GridPane();
        numberGrid.setHgap(5);
        numberGrid.setVgap(5);

        GridPane operatorGrid = new GridPane();
        operatorGrid.setHgap(5);
        operatorGrid.setVgap(5);
/*
        numberGrid.setGridLinesVisible(true);
        operatorGrid.setGridLinesVisible(true);
*/

        ArrayList<Button> numberButtons = new ArrayList<>();
        ArrayList<Button> operatorButtons = new ArrayList<>();

        int buttonCounter = 0;
        int buttonWidth = 500;
        int buttonHeight = 500;
        for(int y = 0; y < 2; y++){
            RowConstraints row = new RowConstraints();
            row.setPercentHeight(20);
            numberGrid.getRowConstraints().add(row);
            for(int x = 0; x < 5 && buttonCounter < 10; x++, buttonCounter++){
                if(y == 0){
                    ColumnConstraints column = new ColumnConstraints();
                    column.setPercentWidth(33);
                    column.setHalignment(HPos.CENTER);
                    numberGrid.getColumnConstraints().add(column);
                }
                Button button = new Button(String.valueOf(buttonCounter));
                button.setPrefWidth(buttonWidth);
                button.setPrefHeight(buttonHeight);
                numberButtons.add(button);
                numberGrid.add(button, x, y);
            }
        }
        String[][] operatorTextArr = {{"+", "-"}, {"*", "/"}, {"=", "c"}};
        for(int y = 0; y < operatorTextArr.length; y++){
            RowConstraints row = new RowConstraints();
            row.setPercentHeight(20);
            operatorGrid.getRowConstraints().add(row);
                for(int x = 0; x < operatorTextArr[y].length; x++){
                    if(y == 0){
                        ColumnConstraints column = new ColumnConstraints();
                        column.setPercentWidth(33);
                        column.setHalignment(HPos.CENTER);
                        operatorGrid.getColumnConstraints().add(column);
                    }
                    Button button = new Button(operatorTextArr[y][x]);
                    button.setPrefWidth(buttonWidth);
                    button.setPrefHeight(buttonHeight);
                    operatorButtons.add(button);
                    operatorGrid.add(button, y, x);
                }
        }

        //Makes clicking the number buttons add to the expressionLabel
        for(int counter = 0; counter < numberButtons.size(); counter++){
            numberButtons.get(counter).setOnAction(new EventHandler<ActionEvent>(){
                public void handle(ActionEvent e){
                    //Gets info about the clicked button
                    Button temp = (Button)e.getSource();
                    if(scriptExceptionOccurred){
                        /*
                        Clears the label once the user tries to enter a new expression
                        after an exception occurred. Can't be done in catch 'cause, well,
                        we need to make sure the user sees the message
                        */
                        expressionLabel.setText("");
                        scriptExceptionOccurred = false;
                    }
                    String newexpressionText = expressionLabel.getText() + temp.getText();
                    expressionLabel.setText(newexpressionText);
                    System.out.println(newexpressionText);
                    //Adds the number on the button to the expression
                    if(operatorAlreadyPressed && !secondOperand){
                        /*
                        If any non-digit button is pressed, this tells the program
                        to evaluate the expression
                        */
                        secondOperand = true;
                    }
                }
            });
        }

        for(int counter = 0; counter < operatorButtons.size(); counter++){
            operatorButtons.get(counter).setOnAction(new EventHandler<ActionEvent>(){
                public void handle(ActionEvent e){
                    String text = expressionLabel.getText();
                    /*Same as above section*/
                    Button temp = (Button)e.getSource();
                    if(scriptExceptionOccurred){
                        expressionLabel.setText("");
                        scriptExceptionOccurred = false;
                    }
                    /*
                    If the clear button is pressed, and the expression has content
                    delete a character
                    */
                    if(temp.getText().equals("c") && expressionLabel.getText().length() > 0){
                        char charToDelete = text.charAt(text.length() - 1);
                        if(charToDelete == '+' || charToDelete == '-'
                            || charToDelete == '*' || charToDelete == '/'){
                            /*
                            Allows an operator to be pressed again if an
                            operator character is deleted
                            */
                            operatorAlreadyPressed = false;
                        }
                        expressionLabel.setText(expressionLabel.getText().substring(
                            0, expressionLabel.getText().length()-1));
                    }
                    /*
                    Only 1 operator is allowed in an expression. If there is none, and the operator pressed != "="
                    add it to expression
                    */
                    else if(!operatorAlreadyPressed && text.length() > 0 && !temp.getText().equals("=")){
                        String newexpressionText = expressionLabel.getText()
                            + temp.getText();
                        expressionLabel.setText(newexpressionText);
                        operatorAlreadyPressed = true;
                    }
                    //If there is another operator already, or the operator pressed == "=", evaluate expression
                    else if(secondOperand || temp.getText().equals("=")){
                        operatorAlreadyPressed = false;
                        secondOperand = false;
                        try{
                            ScriptEngineManager mgr = new ScriptEngineManager();
                                ScriptEngine engine = mgr.getEngineByName("JavaScript");
                            expressionLabel.setText(String.valueOf(engine.eval(
                                expressionLabel.getText())));
                            /*
                            If the expression is evaluated because an operator was already there,
                            and the new operator != "=" and != "c", add the new operator to the 
                            new expression
                            */
                            if(!temp.getText().equals("=") && !temp.getText().equals("c")){
                                expressionLabel.setText(expressionLabel.getText()
                                 + temp.getText());
                                operatorAlreadyPressed = true;
                            }
                        }
                        catch(ScriptException exc){
                            //Basically let the user know and reset important booleans.
                            expressionLabel.setText("Invalid operation.");
                            scriptExceptionOccurred = true;
                            secondOperand = false;
                            operatorAlreadyPressed = false;
                        }
                    }
                }
            });
        }

        root.getChildren().addAll(expressionLabel, numberGrid, operatorGrid);

        primaryStage.setScene(scene);
        primaryStage.sizeToScene();
        primaryStage.show();
    }
}
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Good on you for taking on a project to learn, and even more for posting it here! Keep it up, you'll go far.

Make use of methods

Likely what you mean when you say modular, you should be using more methods. Right now pretty much all your logic is in your start method. Not only would using methods increase both maintainability and readability it would also make adding features more streamlined thus making your entire program both more flexible and extensible.

Here's a small example of where you can do this: This if series where you currently check whether a deleted character is an operator:

if(charToDelete == '+' || charToDelete == '-'
 || charToDelete == '*' || charToDelete == '/'){
     operatorAlreadyPressed = false;
}

You can extract this into the following:

private boolean isOperator(char potentialOperator) {
        return potentialOperator == '+' || potentialOperator == '-' ||
               potentialOperator == '*' || potentialOperator == '/';
}

This way you reduce your if condition to isOperator(charToDelete), You could even have a method that calls this one and alters operatorAlreadyPressed as a result. Try to see where else you can add more methods to your code!

Use Lambda expressions

Unless you're explicitly constrained in which runtime you may use, consider replacing your anonymous inner classes with lambda expressions.

e.g. Everywhere you use the setOnAction method, like the following:

numberButtons.get(counter).setOnAction(new EventHandler<ActionEvent>(){
                public void handle(ActionEvent e){
                // logic
                }
   }

You can write it simply:

numberButtons.get(counter).setOnAction(e -> {
               // logic
});

Read more about Lambda Expressions, from Oracle, here & for a general rundown of Java 8 features I recommend this.

Interface Intuitiveness

The 'C' button typically acts as a clear option on calculator. Yours currently acts as a backspace and you actually lack a clear option altogether. In addition, after any result is displayed any proceeding input appends to the end of the previous result. Lastly, I would organize things to be more like a natural calculator. It's fairly horizontal and there's quite a lot of space that just doesn't do anything. Good work on the flexible sizing however.

Remove commented code.

Commented is dead code. 'Nuff said.

On XML & CSS

A large appeal of JavaFX is the possibility to use CSS stylings and the ability to use XML to separate your model / view from your application logic. There is even software, like Scene Builder, that feature GUI whose use generates the desired XML for an interface. Here's a simple example using both CSS and XML facilitated by Scene Builder to design an interface.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks for the advice. So, what you're saying is that SceneBuilder pretty much makes the GUI design part of my code completely redundant? Also, thanks for mentioning lambdas and showing me how I can clean up that if condition a bit. My original code looked ugly to me, but I wasn't sure how to fix it :) \$\endgroup\$
    – Sky
    Jan 29, 2016 at 15:02
  • 1
    \$\begingroup\$ My pleasure. As for scene builder making your code redundant. Not necessarily. Just a different way to do things. I practiced a lot with designs via code before I touched XML myself. \$\endgroup\$
    – Legato
    Jan 29, 2016 at 15:12
  • \$\begingroup\$ Are there any resources you'd recommend for FXML? \$\endgroup\$
    – Sky
    Jan 30, 2016 at 20:34

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.