Spliting Greeter class to multiple small classes

Posted on

Problem

  1. Write a Greeter class with greet function that receives a name as input and outputs Hello <name>. The signature of greet should not change throughout the kata. You are allowed to construct Greeter object as you please.
  2. greet trims the input
  3. greet capitalizes the first letter of the name
  4. greet returns Good morning <name> when the time is 06:00-12:00
  5. greet returns Good evening <name> when the time is 18:00-22:00
  6. greet returns Good night <name> when the time is 22:00-06:00

Original code

To this kata I provided this code.

public class Greeter {
    private final TimeProvider timeProvider;

    public Greeter() {
        timeProvider = LocalTime::now;
    }

    public Greeter(TimeProvider timeProvider) {
        this.timeProvider = timeProvider;
    }

    public String greet(String name) {
        String beginning = getGreetingBeginning();
        String correctedName = getCorrectedName(name);

        return String.format("%s %s", beginning, correctedName);
    }

    private String getGreetingBeginning() {
        if (isMorning())
            return "Good morning";
        if (isEvening())
            return "Good evening";
        if (isNight())
            return "Good night";
        return "Hello";
    }

    private boolean isMorning() {
        LocalTime actualTime = timeProvider.provide();
        return actualTime.isAfter(LocalTime.parse("06:00:00"))
                && actualTime.isBefore(LocalTime.parse("12:00:01"));
    }

    private boolean isEvening() {
        LocalTime actualTime = timeProvider.provide();
        return actualTime.isAfter(LocalTime.parse("18:00:00"))
                && actualTime.isBefore(LocalTime.parse("22:00:01"));
    }

    private boolean isNight() {
        LocalTime actualTime = timeProvider.provide();
        boolean isAfterEvening = actualTime.isAfter(LocalTime.parse("22:00:00"));
        boolean beforeMidnight = actualTime.isBefore(LocalTime.parse("23:59:59"));
        boolean isJustBeforeMidnight = actualTime.equals(LocalTime.parse("23:59:59"));
        boolean isMidnight = actualTime.equals(LocalTime.parse("00:00:00"));
        boolean isAfterMidnight = actualTime.isAfter(LocalTime.parse("00:00:00"));
        boolean beforeMorning = actualTime.isBefore(LocalTime.parse("06:00:01"));
        return
            (isAfterEvening && beforeMidnight)
            || isJustBeforeMidnight
            || isMidnight
            || (isAfterMidnight && beforeMorning);
    }

    private String getCorrectedName(String name) {
        String trimmedName = name.trim();
        return StringUtils.capitalize(trimmedName);
    }

}
interface TimeProvider {
    LocalTime provide();
}

Code after refactoring

After this I thought that Greeter does to many things so I splitted it to multiple small classes.

public class Greeter {
    private final BeginningProvider beginningProvider;

    public Greeter(BeginningProvider beginningProvider) {
        this.beginningProvider = beginningProvider;
    }

    public String greet(String name) {
        String beginning = beginningProvider.provide();
        String correctedName = getCorrectedName(name);

        return String.format("%s %s", beginning, correctedName);
    }

    private String getCorrectedName(String name) {
        String trimmedName = name.trim();
        return StringUtils.capitalize(trimmedName);
    }
}
interface BeginningProvider {
    String provide();
}
class BeginningProviderImpl implements BeginningProvider {
    private final TimeProvider timeProvider;
    private final List<TimeRangePredicateSupplier> timeRanges = List.of(
        new MorningPredicateSupplier(),
        new EveningPredicateSupplier(),
        new NightPredicateSupplier()
    );

    BeginningProviderImpl() {
        timeProvider = LocalTime::now;
    }

    BeginningProviderImpl(TimeProvider timeProvider) {
        this.timeProvider = timeProvider;
    }

    @Override
    public String provide() {
        LocalTime time = timeProvider.provide();
        for (TimeRangePredicateSupplier predicateSupplier : timeRanges)
            if (predicateSupplier.test(time))
                return predicateSupplier.get();

        return new AfternoonPredicateSupplier().get();
    }
}
interface TimeProvider {
    LocalTime provide();
}
interface TimeRangePredicateSupplier {
    boolean test(LocalTime time);
    String get();
}
class MorningPredicateSupplier implements TimeRangePredicateSupplier {
    @Override
    public boolean test(LocalTime time) {
        return time.isAfter(LocalTime.parse("06:00:00"))
                && time.isBefore(LocalTime.parse("12:00:01"));
    }

    @Override
    public String get() {
        return "Good morning";
    }
}
class EveningPredicateSupplier implements TimeRangePredicateSupplier {
    @Override
    public boolean test(LocalTime time) {
        return time.isAfter(LocalTime.parse("18:00:00"))
                && time.isBefore(LocalTime.parse("22:00:01"));
    }

    @Override
    public String get() {
        return "Good evening";
    }
}
class NightPredicateSupplier implements TimeRangePredicateSupplier {
    @Override
    public boolean test(LocalTime time) {
        boolean isAfterEvening = time.isAfter(LocalTime.parse("22:00:00"));
        boolean beforeMidnight = time.isBefore(LocalTime.parse("23:59:59"));
        boolean isJustBeforeMidnight = time.equals(LocalTime.parse("23:59:59"));
        boolean isMidnight = time.equals(LocalTime.parse("00:00:00"));
        boolean isAfterMidnight = time.isAfter(LocalTime.parse("00:00:00"));
        boolean beforeMorning = time.isBefore(LocalTime.parse("06:00:01"));
        return
                (isAfterEvening && beforeMidnight)
                        || isJustBeforeMidnight
                        || isMidnight
                        || (isAfterMidnight && beforeMorning);
    }

    @Override
    public String get() {
        return "Good night";
    }
}
class AfternoonPredicateSupplier implements TimeRangePredicateSupplier {
    @Override
    public boolean test(LocalTime time) {
        throw new AfternoonCheckException("Afternoon is default case, shouldn't be checked");
    }

    @Override
    public String get() {
        return "Hello";
    }
}
class AfternoonCheckException extends RuntimeException {
    AfternoonCheckException(String s) {
        super(s);
    }
}

Question

Which of the above version is easier to read and which version would you like to see if you just joined to project?

Solution

TimeRangePredicateSupplier

To me, this name seems wrong. The implementations are providing a time based greeting, so perhaps TimeBasedGreetingSupplier might be better.

Afternoon

The way you’ve coded AfternoonPredicateSupplier it knows too much about how it’s used. Whilst the afternoon doesn’t have a particular requirement, since you’ve coded it, it would seem to make sense for it to check a time period that you’ve designated as ‘afternoon’. This would be clearer and it would mean that all of your suppliers implement the interface correctly, which could mean they are useful in other scenarios.

Taking this approach with with Afternoon also simplifies the logic in your Beginning provider. You no longer need to treat Afternoon as a separate case, you just find the first predicate that matches the current time. So, provide can become:

@Override
public String provide() {
    LocalTime time = timeProvider.provide();
    return timeRanges.stream()
            .filter(p -> p.test(time))
            .findFirst()
            .map(TimeBasedGreetingSupplier::get)
            .orElseThrow(() -> new RuntimeException("Invalid time"));
}

Use the power of Spring

Consider letting Spring do some of the heavy lifting for you. If you mark your Predicate classes with a @Service attribute, then you can get Spring to automatically inject a list of all of the classes that implement your Predicate interface. This moves the knowledge about what predicates there are out of your BegginingProviderImpl, so you can and do have extra divisions in the future (EarlyMorning, LateEvening etc) without having to revisit that class.

BeginningProviderImpl(TimeProvider timeProvider, List<TimeBasedGreetingSupplier> predicates) {
    this.timeProvider = timeProvider;
    this.timeRanges = predicates;
}

Committing on Red

I had a brief look through some of the commits on your Repo. You seem to have performed a couple of commits on Red. This should generally be avoided, you should only be committing on Green… unless you’re committing as a way of sharing your approach to solving the kata.

Time Provider

Rather than having a default constructor for BeggingingProviderImpl which sets up the default behaviour, consider defining a bean that spring can inject automatically.

@Service
class DefaultTimeProvider implements TimeProvider {
    @Override
    public LocalTime provide() {
        return LocalTime.now();
    }
}

For simplicity, there’s a fork of your repo here, with some of the suggested changes in it, including changes to the Config classes which you haven’t posted as part of your question.

In the original code you call timeProvider.provide() several times resulting in (slightly) different times in a case where you should be comparing the same time each time. While this may not be problematic in this specific case, it could lead to very difficult to track bugs.


Then in both cases you have over-thought the time comparison. LocalTime has a high resolution of (theoretically) nanoseconds. This leads to a one second gap in the isNight comparison. You could counter this by using .truncatedTo(ChronoUnit.SECONDS), but that isn’t necessary, especially not the whole midnight thing. Just using

time.isAfter(LocalTime.parse("22:00:00")) || time.isBefore(LocalTime.parse("06:00:00"));

will work fine. You also don’t need the one second addition at the end of the other time ranges.

If you want to be very sure, that there is no (theoretical) gap between the evening and night ranges, add

|| time.equals(LocalTime.parse("22:00:00"))

to one of the comparison expressions.


BTW, all the LocalTime.parses are executed on each call. It would be better to use constants to store those values.


One thing this exercise wants to teach, is to make the code easy to extend. Currently you have hard-coded the time ranges and greetings. The second solution with the TimeRangePredicateSupplier is a good start, but the actual implementations are still hard-coded inside BeginningProviderImpl.

In case of the first solution, try implementing it with following constructor:

public Greeter(TimeProvider timeProvider, List<TimeRangeGreeting> timeRangeGreetings, String defaultGreeting) {
    // ...
}

where TimeRangeGreeting is a data class such as

public class TimeRangeGreeting {
   private final LocalTime start;
   private final LocalTime end;
   private final String greeting;

   // TODO constructor, getters
}

or if you are using Java 14 a record:

public record TimeRangeGreeting(LocalTime start, LocalTime end, String greeting) {}

Leave a Reply

Your email address will not be published. Required fields are marked *