3
\$\begingroup\$

This is a simple student project that calculates the return on an investment with a given investment amount, number of years invested, and annual interest rate. I know this is pretty basic, but I'm just looking for feedback for formatting and other general improvements I can make to the code.

// Author: Joshua Ferrell
// Date: 3/25/2017

import javafx.application.Application;
import javafx.geometry.Pos;
import javafx.geometry.HPos;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.control.Label;
import javafx.scene.control.TextField;
import javafx.scene.layout.GridPane;
import javafx.stage.Stage;

// A program that caculates the return on an investment.

public class Exercise15_5 extends Application {

    // globals
    private TextField tfInvestmentAmount = new TextField();
    private TextField tfNumYears = new TextField();
    private TextField tfAnnualInterestRate = new TextField();
    private TextField tfFutureValue = new TextField();
    private Button btCalc = new Button("Calculate");

    @Override
    public void start(Stage primaryStage) {

        // Create UI
        GridPane gridPane = new GridPane();
        gridPane.setHgap(5);
        gridPane.setVgap(5);
        gridPane.add(new Label("Investment Amount:"), 0, 0);
        gridPane.add(tfInvestmentAmount, 1, 0);
        gridPane.add(new Label("Number of Years:"), 0, 1);
        gridPane.add(tfNumYears, 1, 1);
        gridPane.add(new Label("Annual Interest Rate:"), 0, 2);
        gridPane.add(tfAnnualInterestRate, 1, 2);
        gridPane.add(new Label("Future Value:"), 0, 3);
        gridPane.add(tfFutureValue, 1, 3);
        gridPane.add(btCalc, 1, 4);

        // Set Properties for UI
        gridPane.setAlignment(Pos.CENTER);
        tfInvestmentAmount.setAlignment(Pos.BOTTOM_RIGHT);
        tfNumYears.setAlignment(Pos.BOTTOM_RIGHT);
        tfAnnualInterestRate.setAlignment(Pos.BOTTOM_RIGHT);
        tfFutureValue.setAlignment(Pos.BOTTOM_RIGHT);
        tfFutureValue.setEditable(false);
        gridPane.setHalignment(btCalc, HPos.RIGHT);

        // Process event (caculate future value)
        btCalc.setOnAction(e -> calculateFutureValue());

        // Create a scene and place it in the stage
        primaryStage.setScene(new Scene(gridPane, 300, 200));
        primaryStage.setTitle("Exercise15_05"); // Set Title
        primaryStage.show(); // Display the Stage
    } 

    private void calculateFutureValue() {

        // Get Values from text fields
        double amount = Double.parseDouble(tfInvestmentAmount.getText());
        double interest = Double.parseDouble(tfAnnualInterestRate.getText());
        int years = Integer.parseInt(tfNumYears.getText());

        // Get Monthly interest rate
        double monthlyInterestRate = interest / 12 / 100;

        // Get Future Value
        double futureValue = amount * Math.pow(1 + monthlyInterestRate, years * 12);

        // set futureValue to tfFutureValue
        tfFutureValue.setText(String.format("$%.2f", futureValue));
    }
}
\$\endgroup\$
2
\$\begingroup\$

The first thing that is noticeable when looking at this code is that there are too many comments. Let me explain.

Example 1 - stating the obvious

    // set futureValue to tfFutureValue
    tfFutureValue.setText(String.format("$%.2f", futureValue));

This comment does not add any value to your program, it creates mess. The code itself is self-explanatory, and even if I had 2 weeks experience in Java, I would know what it does.

Example 2 - stating the untrue

public class Exercise15_5 extends Application {

    // globals
    private TextField tfInvestmentAmount = new TextField();
    private TextField tfNumYears = new TextField();
    private TextField tfAnnualInterestRate = new TextField();

The comment that says //globals is simply wrong - the TextFields are private fields of Exercise15_5 class. You can create a "global" variable in Java by creating a public static field - then you can use it everywhere, but of course you don't want to do that in your application

Example 3 - extract code to a method instead of commenting

In your start() method, you have 4 big blocks of code that are doing different things and are commented accordingly, like:

    // Set Properties for UI
    gridPane.setAlignment(Pos.CENTER);
    tfInvestmentAmount.setAlignment(Pos.BOTTOM_RIGHT);
    tfNumYears.setAlignment(Pos.BOTTOM_RIGHT);
    tfAnnualInterestRate.setAlignment(Pos.BOTTOM_RIGHT);
    tfFutureValue.setAlignment(Pos.BOTTOM_RIGHT);
    tfFutureValue.setEditable(false);
    gridPane.setHalignment(btCalc, HPos.RIGHT);

We can extract those lines to a new method and give it some meaningful name... Just like we would comment those lines!

private void setPropertiesForUI() {
    gridPane.setAlignment(Pos.CENTER);
    tfInvestmentAmount.setAlignment(Pos.BOTTOM_RIGHT);
    tfNumYears.setAlignment(Pos.BOTTOM_RIGHT);
    tfAnnualInterestRate.setAlignment(Pos.BOTTOM_RIGHT);
    tfFutureValue.setAlignment(Pos.BOTTOM_RIGHT);
    tfFutureValue.setEditable(false);
    gridPane.setHalignment(btCalc, HPos.RIGHT);
}

Then, your start() method becomes much cleaner:

@Override
public void start(Stage primaryStage) {
    createUI();
    setPropertiesForUI();
    calculateFutureValue();
    createSceneAndPlaceItInTheStage();
}

Split logic and UI into separate classes

I also believe that the calculateFutureValue() method should not be in the Exercise15_5 class. It should be extracted to another class, possibly something named like InvestmentCalculator. The method should take doubles and int as input parameters, not Strings (it doesn't know about textfields and UI).

You can make the calculateFutureValue() static in the InvestmentCalculator class since it's just performing some calculations and returning a value - you don't need an instance of it. You can even make InvestmentCalculators constructor private to prevent creating an instance of it - in other words, making the InvestmentCalculator a Util Class.

I don't want to go into the implementation details since this is tagged as homework, but you can always edit your question or post another one after the refactoring.

\$\endgroup\$

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.