Calculation of the money to be paid back to the employer

Question

I'm a beginner developer and I wrote a simple program. How could be the code improved to meet best practices of the development? I mean if the names of classes or variables are enough self-explaining for others and it is required to insert setters and getters in the classes, when I know that at this point it is necessary? Any help to improve the code would be appreciated.

Brief code explanation

An employee signs a contract with an employer, that he cannot terminate before 18 months working there. Otherwise he is obliged to pay back a certain amount of money, that decrease each month of working.

UPDATE

Assuming a user of the program started a job on 1st April 2017, he inserts in the Main class a date of starting a potential new job. In the CalculateAmountOfMonthToGo class determines how many months differs the inserted date and the date that he does not need to pay back any money. Then in the class CalculateAmountOfMoneyToBePaid it is calculated how much he needs to repay. Later there will be displayed a kind of summary with dates of starting new job and the amount of money to be repaid at that time.

Main.java

package changeWork;

import java.text.DecimalFormat;

public class Main {

    public static void main(String[] args) {
        CalculateAmountOfMonthToGo monthsToGo = new CalculateAmountOfMonthToGo("01-10-2017");
        monthsToGo.retriveAmountOfMonthToGo();

        CalculateAmountOfMoneyToBePaid moneyToBePaidClass = new CalculateAmountOfMoneyToBePaid();
        double money=moneyToBePaidClass.amountMoneyToPayBack();
        DecimalFormat df = new DecimalFormat("#.##");

        System.out.println("Do splaty pozostalo:");
        System.out.println(df.format(money));

        System.out.println();
        System.out.println("Kwota do zaplaty po kolejnych miesiacach");
        moneyToBePaidClass.printMoneyToBePaidEachMonth();

    }

}

CalculateAmountOfMonthToGo.java

package changeWork;

import java.time.LocalDate;
import java.time.format.DateTimeFormatter;
import java.time.temporal.ChronoUnit;
public class CalculateAmountOfMonthToGo {


    private final String END_PENALTY="01-10-2018"; 
    private LocalDate newWorkStartDate;
    private String newWorkStart;
    private long monthsBetween=0;

    public String getNewWorkStart() {
        return newWorkStart;
    }

    public void setNewWorkStart(String newWorkStart) {
        this.newWorkStart = newWorkStart;
    }

    public long getMonthsBetween() {
        return monthsBetween;
    }

    public void setMonthsBetween(long monthsBetween) {
        this.monthsBetween = monthsBetween;
    }

    public CalculateAmountOfMonthToGo(String newWorkStart) {
        super();
        this.newWorkStart = newWorkStart;
    }
    public CalculateAmountOfMonthToGo(){

    }

    public long retriveAmountOfMonthToGo(){
         DateTimeFormatter formatter = DateTimeFormatter.ofPattern("dd-MM-yyyy");
         try{
         newWorkStartDate =LocalDate.parse(newWorkStart, formatter);}
         catch (Exception e) {
            e.printStackTrace();
        }
         LocalDate endPenaltyDate= LocalDate.parse(END_PENALTY,formatter);
         monthsBetween= ChronoUnit.MONTHS.between(newWorkStartDate, endPenaltyDate);
         if (monthsBetween<0){
             throw new RuntimeException("Rozpoczecie pracy po wygasnieciu okresu splaty");
         } else if (monthsBetween>18){
             throw new RuntimeException("Bledna data rozpoczecia nowej pracy");
         }
        return monthsBetween;
    }   

}

CalculateAmountOfMoneyToBePaid.java

package changeWork;

import java.text.DecimalFormat;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;

public class CalculateAmountOfMoneyToBePaid {
    private final double PENALTY = 6000;
    private final double PENALTY_DURATION = 18;
    private String newWorkStart;
    private long monthsBetween = 0;
    private HashMap<String, Double> amountOfMoney = new HashMap<String, Double>();

    public CalculateAmountOfMoneyToBePaid() {
    }

    public CalculateAmountOfMoneyToBePaid(long monthsBetween) {
        this.monthsBetween = monthsBetween;
    }

    public double amountMoneyToPayBack() {
        double ratio = monthsBetween / PENALTY_DURATION;

        return ratio * PENALTY;
    }

    public HashMap<String, Double> printMoneyToBePaidEachMonth() {
        CalculateAmountOfMonthToGo monthsToGo = new CalculateAmountOfMonthToGo();
        System.out.println("Data: \t\t Kwota:");
        List<String> keys = Arrays.asList("01-04-2017", "01-05-2017", "01-06-2017", "01-07-2017", "01-08-2017",
                "01-09-2017", "01-10-2017", "01-11-2017", "01-12-2017", "01-01-2018", "01-02-2018", "01-03-2018",
                "01-04-2018", "01-05-2018", "01-06-2018", "01-07-2018", "01-08-2018", "01-09-2018");

        for (int i = 0; i < keys.size(); i++) {
            newWorkStart = keys.get(i);
            monthsToGo.setNewWorkStart(newWorkStart);
            monthsBetween = monthsToGo.retriveAmountOfMonthToGo();
            double moneyToPay = amountMoneyToPayBack();
            amountOfMoney.put(newWorkStart, moneyToPay);
            DecimalFormat df = new DecimalFormat("#.##");
            System.out.println(newWorkStart + "\t\t" + df.format(moneyToPay));

        }

        return amountOfMoney;
    }

}

Answer 1

• Avoid strings where other types are more appropriate. You can use Date objects instead and eliminate some of the overhead of converting them.

• Rather than use a hard-coded list of dates (List<String> keys), generate them dynamically.

• I don't think you need three classes to achieve the functionality you want. Use a minimal Main class to launch "PenaltyCalculator" class to do all the work.

• "CalculateAmountOfMonthsToGo" sounds like the name of a method. It should be a method in the new "PenaltyCalculator" instead of its own class. As a method, it can take newWorkStart as a parameter.
• Likewise, "CalculateAmountOfMoneyToBePaid" should be a method in PenaltyCalculator.
• A separate "printPenalties()" method can iterate over the months and call the other two methods with each iteration. The constants like private final double PENALTY = 6000; and private final double PENALTY_DURATION = 18; can live in the printPenalties() method. This method is called from Main.
• Now for the reach: Create a test harness so that you can quickly check that you haven't changed the answers as you refactor your program. Only preaching this because you said you were new to programming. Earlier you get used to test driven development, the more productive you'll be.

UPDATE: The following code reduces the lines of code from over 100 to about 30 by getting rid of the List called keys as well as the method or class called CalculateAmountOfMonthToGo (it is taken care of in the for loop). This eliminates the need to convert Strings to Dates and does not need the HashMap named amountOfMoney as this is calculated on the fly in each iteration of the for loop:

import java.time.LocalDate;
import java.text.DecimalFormat;
public class PenaltyCalculator{
    private final double PENALTY = 6000;
    private final double PENALTY_DURATION = 18; 

    public void printPenaltiesByMonth(){
        // start day is first day of current month:
        LocalDate today = LocalDate.now();
        LocalDate monthStart = today.withDayOfMonth(1);
        System.out.println("Quit On    | Owe This");
        //duration is 18 months, so program should loop 18 times:
        for(int monthsOnTheJob = 0; monthsOnTheJob < PENALTY_DURATION; monthsOnTheJob++){
            DecimalFormat df = new DecimalFormat("#.##");

            System.out.println(monthStart.plusMonths(monthsOnTheJob) + " : " + df.format(calculatePenalty(PENALTY_DURATION-monthsOnTheJob)));

        }
    }


    public double calculatePenalty(double monthsRemaining){
        double ratio = monthsRemaining / PENALTY_DURATION;
        return ratio * PENALTY;
    }


}

and this, then, is the Main.java class that just serves to call the above:

public class Main {
    public static void main(String[] args) {
        PenaltyCalculator penaltyCalc = new PenaltyCalculator();
        penaltyCalc.printPenaltiesByMonth();
 }
}

Answer 2

Thank you for sharing your code!

Naming

Finding good names is the hardest part in programming. So always take your time to think about your identifier names.

On the bright side you follow the Java Naming Conventions.

But some of your names are brittle.

• Classes and variables should be named like nouns
• Methods should be named like verbs
• Interfaces should be named like adjectives
• Names of variables holding a boolean and methods returning a boolean should start with "is" or "has" (or maybe "can", "will").

In general the identifiers should express your intent. The should be chosen from the problem domain, not from the technical solution. Eg. look at

 double money=moneyToBePaidClass.amountMoneyToPayBack();

_{following names suggested are wild guesses...} - The variable name money does mean nothing in respect to your program. A better name might be totalSum. - The variable name moneyToBePaidClass might better be interestCalculator. - The method name amountMoneyToPayBack() should be calculateTtotalSum().

This would turn the line into:

 double totalSum = interestCalculator.calculateTotalSum();

which makes it much more understandable.

use dependency injection

Your class CalculateAmountOfMoneyToBePaid has a dependency to your other class CalculateAmountOfMonthToGo. Since you have your class CalculateAmountOfMoneyToBePaid instantiate CalculateAmountOfMonthToGo itself this dependency is hidden.

I for myself consider "hidden instantiation" of dependencies as a violation of the Single Responsibility Pattern: The instantiation is a separate Responsibility and should not be done by the object using them.

You should pass an instance of CalculateAmountOfMonthToGo as constructor parameter toCalculateAmountOfMoneyToBePaid and store that in a finalmember variable.

apply the MVC pattern

Any application consists of 3 layers:

1. The Model which represents the applications state. This is the bottom layer. It does not have business logic. Only some infrastructure to notify about changes of data who ever wants to know it...

2. The Controller which provides logic working on the model, changing the applications state. This is the medium layer and obviously knows the model. It does not know (and therefore not change) the view.

3. The View which displays the applications state and handles user input delegating it to the controller. It knows the model and display data from it. It also knows the controller and uses it to change data in the model.

In your approach you're mixing this layers withing the same methods/objects.

is required to insert setters and getters in the classes?

In opposite it is discouraged since it violates the most important OO principle information hiding / encapsulation.

Getters and getters have their justification in Data Transfer Objects (DTOs) or ValueObjects which are dump data containers and have no business logic of their own.

Other classes should not have setters and getters.

Answer 3

Naming

CalculateAmountOfMonthToGo is a class. It holds information. It itself doesn't calculate anything. It is something. Perhaps MonthsToGoCalculator, but I would just call it MonthsToGo. Or something like TimeRemaining.

Same thing with CalculateAmountOfMoneyToBePaid. Could be MoneyToBePaid. Or something like BillingRecord.

Scope

    private String newWorkStart;

You only use this in one method. In fact, you only use it one loop.

        for (int i = 0; i < keys.size(); i++) {
            newWorkStart = keys.get(i);

This could be

        for (String newWorkStart : keys) {

You never use i for anything except getting a value. So you might as well use the for-each form and avoid i entirely.

This limits the scope of the variable to the smallest possible. So the object to which it points can be garbage collected when not needed, and it doesn't interfere with other variables that you might want to give the same name.

Interfaces over implementations

    private HashMap<String, Double> amountOfMoney = new HashMap<String, Double>();

Here, a HashMap is an implementation.

    private Map<String, Double> amountOfMoney = new HashMap<>();

The interface is Map. Now, if we wanted to switch implementations, we could do so just here. Also, we'll be pushed to limit our use of amountOfMoney to just the methods in the interface.

In newer versions of Java, we don't have to repeat the String, Double part. If we leave the second blank, the compiler will fill in the right values.

As a general rule (which of course has exceptions), we only use implementations at initialization time. For the type, we use the interface. This leaves things more flexible in the face of future uses of the class or method.