4
\$\begingroup\$

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;
    }

}
\$\endgroup\$
  • 2
    \$\begingroup\$ As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Vogel612 Jul 6 '17 at 9:42
  • 1
    \$\begingroup\$ Thank you for your suggestion. I had read it and I made some improvements. \$\endgroup\$ – Krzychu Jul 6 '17 at 10:00
4
\$\begingroup\$
  • 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();
 }
}
\$\endgroup\$
  • \$\begingroup\$ Instead of the DecimalFormat, you might as well write System.out.printf("%s : %7.2f%n", monthStart.plusMonths(monthsOnTheJob), calculatePenalty(PENALTY_DURATION - monthsOnTheJob));. \$\endgroup\$ – Roland Illig Jul 7 '17 at 7:02
  • \$\begingroup\$ All in all, the number 18 should only appear once in the whole program. \$\endgroup\$ – Roland Illig Jul 7 '17 at 7:03
  • \$\begingroup\$ Roland, two great points. \$\endgroup\$ – mcgyver5 Jul 7 '17 at 11:42
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ MVC is way over the top for such a simple task. \$\endgroup\$ – Roland Illig Jul 7 '17 at 6:53
  • \$\begingroup\$ Good point about naming. I like the idea about using nouns, verbs, adjectives. I read about that convention and forgot, but explaining in this way as you did, makes it easy to remember. Thank you \$\endgroup\$ – Krzychu Jul 7 '17 at 7:26
  • \$\begingroup\$ I'll also try with CDI. Thank you for your suggestions \$\endgroup\$ – Krzychu Jul 7 '17 at 7:32
  • \$\begingroup\$ @RolandIllig "MVC is way over the top for such a simple task." The 3 layers are there anyway. Making them explicit - even with this "simple" task - makes the program both easier to understand and easier to extend. And after all this is a "training" job. There is no reason for being "practical". \$\endgroup\$ – Timothy Truckle Jul 7 '17 at 8:39
1
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Variables don't get garbage collected. Only objects do. \$\endgroup\$ – Roland Illig Jul 7 '17 at 6:50
  • \$\begingroup\$ Good thing to remember is to use interfaces over implementations. I have already seen this in code and I always wondered what was the purpose of doing that. Now I know. Thank you for that. \$\endgroup\$ – Krzychu Jul 7 '17 at 7:40
  • \$\begingroup\$ @RolandIllig I updated to reflect that. \$\endgroup\$ – mdfst13 Jul 7 '17 at 23:29

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.