5
\$\begingroup\$
  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?

\$\endgroup\$
5
  • 2
    \$\begingroup\$ I’m voting to close this question because the code to review is hosted somewhere else. Please take the tour, have a look around, and read through the help center, in particular How do I ask a good question? and What topics can I ask about here?. \$\endgroup\$ May 17, 2021 at 9:15
  • 1
    \$\begingroup\$ TDD is a working method it cannot be evaluated by looking at static outcome of your work. You should look for community events where you can meet other TDD enthusiasts. They will happily code along with you, share best practices and help developing your TDD skills. The most famous event is the Global Day of Code retreat. \$\endgroup\$ May 17, 2021 at 9:20
  • \$\begingroup\$ Welcome to Code Review. As others have mentioned, there are a couple of problems with your question. The code to be reviewed has to be embedded in the question itself, not behind links. \$\endgroup\$
    – Mast
    May 17, 2021 at 12:05
  • \$\begingroup\$ Yes, you are right, this question have a couple of problems. My original thought was that someone would go through commits history and check if my TDD approach was correct, but I see that it is not correct to link code from external source and question does not match CodeReview rules in general. I edited question, I hope that now it does match CodeReview rules. \$\endgroup\$
    – kuch
    May 17, 2021 at 13:02
  • 1
    \$\begingroup\$ "Which of the above version is easier to read" - the first one. I wouldn't be able to understand what the second one does without having read the first. Especially if I had to open multiple files. Also both versions are missing doc comments. \$\endgroup\$
    – Bergi
    May 17, 2021 at 23:54

2 Answers 2

3
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ Beginning provider this is exactly what I originally wanted to achieve, but I didn't know how to do this using streams, that's why I've coded AfternoonPredicateSupplier in this way. Committing on Red Yes, purpose of those commits is to show my approach to solving the kata. Actually, when I was learning Git, I were told "Code that is at not master branches doesn't have to work, even compile, but if you merge your branch with master, it has to work correctly.", that's why I commited on RED. \$\endgroup\$
    – kuch
    May 18, 2021 at 10:08
  • \$\begingroup\$ Spring I agree, that predicates should be dependency of BeginningProvider. Actually I prefer to manually do some of "hard work", such as manually creating some beans in Spring configuration, because then I can easily use these production configured objects in my test code without running Spring context, just exchanging dependencies (IO, time). \$\endgroup\$
    – kuch
    May 18, 2021 at 10:09
  • 1
    \$\begingroup\$ If you want to see my approach for this, here's branch with applied your suggested changes, and file where I use production configured objects in unit tests. Thanks for reviewing my commits! \$\endgroup\$
    – kuch
    May 18, 2021 at 10:10
4
\$\begingroup\$

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) {}

\$\endgroup\$
2
  • \$\begingroup\$ isNight I tried you approach, just looking if actual time is after 22 and before 06, but all of my Good night tests stopped working after this. It seems that when you for example pass 22:00:01, then first condition is met, because 22:00:01 is after 22:00:00, but second condition is not met, because 22:00:01 is not before 06:00:00, but after. Maybe there is a better class for this problem? I agree with rest of your comments \$\endgroup\$
    – kuch
    May 18, 2021 at 10:22
  • 1
    \$\begingroup\$ Oops, I forgot to change the operator there. You need to use || there instead of &&. I'll edit it. \$\endgroup\$
    – RoToRa
    May 18, 2021 at 12:04

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.