4
\$\begingroup\$

To improve my programming style, I have been writing solutions for popular code katas. I have written a partial solution for the Berlin Clock kata and have been trying to follow the SOLID Principles. Can anyone critique the code and suggest improvements?

public class BerlinClock { private static List<TimeUnit> timeUnits = new ArrayList<TimeUnit>(); public static void main(String[] args) { timeUnits.add(new Second()); timeUnits.add(new Hour()); timeUnits.add(new Minute()); System.out.println(getTime("13:17:01")); } public static String getTime(String time) { StringBuilder timeSB = new StringBuilder(); String[] timeElements = time.split(":"); int i=0; int j=2; for(TimeUnit timeUnit : timeUnits){ for(String timeElement : timeElements){ timeSB.append(timeUnits.get(i).getLamps(Integer.parseInt(timeElements[j]))).append("\n"); i++; j--; break; } } return timeSB.toString(); } }

TimeUnit

public interface TimeUnit { public String getLamps(int unit); }

Hour

public class Hour implements TimeUnit { private static final String ALL_LIGHTS_OFF = "OOOO"; private static final String RED_LIGHT = "R"; private static final int MULTIPLE_OF_FIVE = 5; @Override public String getLamps(int hrs) { return getLampsForHoursMultiplesOfFive(hrs) + "\n" + getLampsForSingleHours(hrs); } public String getLampsForHoursMultiplesOfFive(int hours) { StringBuilder lamps = new StringBuilder(ALL_LIGHTS_OFF); for (int i = 0; i < (hours / MULTIPLE_OF_FIVE); i++) { lamps.replace(i, i + 1, RED_LIGHT); } return lamps.toString(); } public String getLampsForSingleHours(int hours) { StringBuilder lamps = new StringBuilder(ALL_LIGHTS_OFF); for (int i = 0; i < (hours % MULTIPLE_OF_FIVE); i++) { //dont do calculaion ever loop, do outside loop lamps.replace(i, i + 1, RED_LIGHT); } return lamps.toString(); } }

Minute

public class Minute implements TimeUnit { private static final String RED_LIGHT = "R"; private static final String YELLOW_LIGHT = "Y"; private static final String ALL_FOUR_LIGHTS_OFF = "OOOO"; private static final String ALL_ELEVEN_LIGHTS_OFF = "OOOOOOOOOOO"; @Override public String getLamps(int minutes) { int minutesDividedBy5 = minutes / 5; int minutesModulus5 = minutes % 5; return getLampsForMinutesMultiplesOfFive(minutesDividedBy5) + "\n" + getLampsForSingleMinutes(minutesModulus5); } private String getLampsForMinutesMultiplesOfFive(int minutes) { StringBuilder lamps = new StringBuilder(ALL_ELEVEN_LIGHTS_OFF); for (int i = 0; i < minutes; i++) { if (0 == (i + 1) % 3) { lamps.replace(i, i + 1, RED_LIGHT); } else { lamps.replace(i, i + 1, YELLOW_LIGHT); } } return lamps.toString(); } private String getLampsForSingleMinutes(int minutes) { StringBuilder lamps = new StringBuilder(ALL_FOUR_LIGHTS_OFF); for (int i = 0; i < minutes; i++) { lamps.replace(i, i + 1, YELLOW_LIGHT); } return lamps.toString(); } }

Second

public class Second implements TimeUnit { @Override public String getLamps(int seconds) { if (0 == seconds%2) { return "Y"; } return "O"; } }
\$\endgroup\$
4
  • 2
    \$\begingroup\$ Perhaps an explanation of what the Berlin Clock kata is would help. \$\endgroup\$ Commented Aug 13, 2014 at 22:15
  • \$\begingroup\$ Isn't this supposed to be a TDD kata? Could you please post your unit tests? \$\endgroup\$ Commented Aug 14, 2014 at 14:12
  • 1
    \$\begingroup\$ Please do not make substantial edits to your question at this point in time. Ask a new one instead. The new question that you tried to ask would however be off-topic for Code Review (and all other Stack Exchange sites), so please don't try to ask it at all. \$\endgroup\$ Commented Nov 22, 2014 at 12:09
  • 1
    \$\begingroup\$ It appears that you have made multiple attempts to deface this question. \$\endgroup\$ Commented Jan 22, 2015 at 20:22

5 Answers 5

Reset to default
6
\$\begingroup\$

That's a lot of code for something that could be done with a single function. The number of operations required to determine the output is relatively small, so writing a whole TimeUnit class hierarchy for it is overkill. Keeping your code short and simple makes it easier to understand, which helps preventing (or spotting) bugs.

If you do want to write a utility class, then a single, configurable TimeUnit class is sufficient. The only difference between the time units are lamp color and number of lamps per row, which can be passed in via the TimeUnit constructor.

Instead of passing strings around, why not use a built-in date/time type? That, or a set of 3 integers. That will clearly communicate how the function is to be used. A string is more ambiguous: should I pass in "15:30:05", "3:30:05PM", or some other format?

The 'S' in SOLID stands for Single responsibility. Let another function do the parsing. Your function should focus on the time-to-BerlinClock-display conversion. That's also related to 'I': Interface segregation. It's better to have several classes (or functions, in this case) that each do one thing well, than one big class/function that does everything, but turns out to be very inflexible when you only need part of that 'everything'.

Remember, KISS: Keep It Short and Simple. Also, DRY: Don't Repeat Yourself.

\$\endgroup\$
5
\$\begingroup\$

Simplifying the main method

The BerlinClock class is very confusing, it would be better to simplify:

class BerlinClock { public static String getTime(String time) { if (!time.matches("\\d\\d:\\d\\d:\\d\\d")) { throw new IllegalArgumentException("Time must be in the format HH:MM:SS"); } StringBuilder timeSB = new StringBuilder(); String[] timeElements = time.split(":"); timeSB.append(new Second().getLamps(Integer.parseInt(timeElements[2]))).append("\n"); timeSB.append(new Hour().getLamps(Integer.parseInt(timeElements[1]))).append("\n"); timeSB.append(new Minute().getLamps(Integer.parseInt(timeElements[0]))).append("\n"); return timeSB.toString(); } }

This is equivalent to your original code, but I think a lot more readable. Now I have a much better idea how a Berlin Clock is supposed to work: first you render the seconds, then the hours, then the minutes.

I also added some input validation to enforce the correct time format, with a helpful message in case the caller doesn't get it right.

If you really want to cache the time units in static variables, give them names. The loop just makes it confusing. You can gain very little from it, and lose a lot in terms of readability.

Unit testing

To verify the clock actually works, add proper unit tests, don't just print in the main method:

@Test public void test_00_00_00() { assertEquals("Y\n" + "OOOO\n" + "OOOO\n" + "OOOOOOOOOOO\n" + "OOOO\n", BerlinClock.getTime("00:00:00")); } @Test public void test_13_17_01() { assertEquals("O\n" + "RRRO\n" + "RROO\n" + "YYOOOOOOOOO\n" + "YYYO\n", BerlinClock.getTime("13:17:01")); }

And let's add all the unit tests from the site that you linked:

@Test public void test_23_59_59() { assertEquals("O\n" + "RRRR\n" + "RRRO\n" + "YYRYYRYYRYY\n" + "YYYY\n", BerlinClock.getTime("23:59:59")); } @Test public void test_24_00_00() { assertEquals("Y\n" + "RRRR\n" + "RRRR\n" + "OOOOOOOOOOO\n" + "OOOO\n", BerlinClock.getTime("24:00:00")); }

You have a bug

Sadly, the last 2 unit tests fail. Let's look at the refactored main method:

timeSB.append(new Second().getLamps(Integer.parseInt(timeElements[2]))).append("\n"); timeSB.append(new Hour().getLamps(Integer.parseInt(timeElements[1]))).append("\n"); timeSB.append(new Minute().getLamps(Integer.parseInt(timeElements[0]))).append("\n");

Thanks to the refactoring it's easier to see something strange: the hour and minute time units use the wrong segment of the input! It seems it should be this way:

timeSB.append(new Second().getLamps(Integer.parseInt(timeElements[2]))).append("\n"); timeSB.append(new Hour().getLamps(Integer.parseInt(timeElements[0]))).append("\n"); timeSB.append(new Minute().getLamps(Integer.parseInt(timeElements[1]))).append("\n");

Interestingly enough, with this change, now only the first unit test fails, the other 3 are passing.

\$\endgroup\$
3
\$\begingroup\$
  • I don't understand the point of the main(). Part of it seems to be necessary setup of the static variable timeUnits but part of it seems to be a test case.
    • My suggestion is to make sure that the static variable is set up properly automatically, without needing to be set up by the tester.
  • And, the tester should be in a separate class from the actual logic.
  • In int i=0; int j=2; what do the variables mean? Why are they initialized to 0 and 2? They're not just plain loop indexes.
  • Why is there a break; in the innermost for loop? Doesn't this counteract the purpose of having a loop?
  • Where's the definition of Second and Minute? The code is supposed to be complete.
  • getLampsForHoursMultiplesOfFive and getLampsForSingleHours are identical except for the transformation applied to hours. Don't repeat yourself. Find some way to factor out the common code.
  • Your comment " do outside loop" will probably be done automatically for you by the compiler/optimizer/JIT.
\$\endgroup\$
1
  • \$\begingroup\$ Second and Minute added. I have now refactored getLampsForHoursMultiplesOfFive into 1 method. \$\endgroup\$ Commented Aug 13, 2014 at 22:17
3
\$\begingroup\$

General Points

It can be considered A Matter Of Personal Preference (AMOPP) (TM) but I would separate the driver (the piece with main()) from the target code. It's a trivial change but speaks to an intent. Here is our nicely encapsulated Berlin Clock, ready for use wherever we want as opposed to, here is a fixer upper that needs some work before it can be used.

There really should be no public methods on a class that implements an interface other than those that support the interface. If the class has other public members they will get used by someone and that kills your use of the interface as a contract. Unit Tests should be performed against the public behaviour of the class. If there is complicated functionality that really needs to be separately tested, then extract it into another class test it there and then embed (or inject) the ancillary class. Testing the implementation makes the tests very brittle.

e.g.

For the 12 cases for hours it is probably most simple to use a look up table to select and return the correct value. It is faster to run (not a great matter here, I know) and simpler to code and review. If there were tests only on the output of the Hour then this change in implementation would not require any changes to the tests but as the code stands there are tests for getLampsForHoursMultiplesOfFive() and getLampsForSingleHours() that need to be removed.

Specific Points
Second
AMOPP but I would use the ternary statement instead of the 'if'.

The values for Yellow (Y) and Off (O) are string literals here but constants in the other TimeUnits. The inconsistency is strange.
On a larger note, Red (R), Yellow (Y) and Off (O) are defined multiple times (Off indirectly because we have "O" in Second and indirectly in ALL_LIGHTS_OFF, ALL_FOUR_LIGHTS_OFF, ALL_ELEVEN_LIGHTS_OFF. They really should be in one place only so that if we decide to change "O" to "-" we can do this without having to hunt for all the usages.

Minute
Lookup for getLampsForSingleMinutes?

Hour
As I said, I would probably go with the look-up here but if not, the structure for both hours and minutes is the same. They both return "X\nY" where X is the count of fives, formatted is some way and Y is the remainder, formatted in some way. A base class that can be extended for both Hour and Minute seems appropriate

public abstract class MajorTimeUnit implements TimeUnit { private static final int Divisor = 5; @Override public String getLamps(int timeUnit) { return getLampsForMultiples(timeUnit / Divisor) + "\n" + getLampsForRemainder(timeUnit % Divisor); } protected abstract String getLampsForMultiples(int value); protected abstract String getLampsForRemainder(int value); }

(Sorry if the code is not quiet right - no compiler here at the moment :? )

getLampsForRemainder() is actually the same code so it be implemented in MajorTimeUnit with say an abstract getter for the lamp colour.

\$\endgroup\$
1
  • \$\begingroup\$ I col'd (chuckled out loud) at AMOPP (TM). \$\endgroup\$ Commented Aug 14, 2014 at 19:39
2
\$\begingroup\$
  • the OO model is good in general, do not listen to these who says it should be one utility method or class.
  • but I would expose only one class/interface- converter, no need to expose all classes (minutes,hours)
  • I would add Lamp model and BerlinClock or BerlinClockConverter model
  • I would not use static runner

  • I would rename getTime() - String time = BerlinClock.get("again string") - doesn't look clear.

    BerlinClock berlinClock = BerlinClock.createInstance("timesastring"); System.out.println (berlinClock.toString());

\$\endgroup\$

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.