Diamond Kata in Java

Someone recently has sent me Nat Pryce’s blog post with his property test driven approach to diamond kata. I had a quick look and decided to try diamond kata myself.

My first try was one huge failure. I wrote 3 tests in form of input – expected output, where the output was entire diamond. Although making the first two pass was quite easy, making the last test pass turned out to be tricky. The TDD iteration cycle was far too long (10+ minutes) and I even had to use debugger. The solution worked, was fully tested, but I was not happy about how difficult it was to create it.

I have read the rest of Nat’s article. His approach is very interesting, however I felt that there is too much logic in tests. Also, a single feature is covered in multiple tests. If someone asked me the question “what does this method return?” I would not be able to answer it easily. I would have to join together these five properties to build a full image. I felt that a test asserting on entire output is necessary, but of course there was something else needed.

After some time I have figured out what is my problem. Normally, when given a large task, we split it into smaller units, use mocking etc. So I though why not to do the same here? When we implement REST endpoint we usually have the acceptance test that makes the request and asserts on the response (end-to-end black-box acceptance test) and a bunch of unit tests testing separately everything that is going on under the hood.

So I have started by creating a single acceptance test:

public class DiamondTest {

    @Test
    public void createsDiamondSevenBySevenForD() {
        List expectedRows = asList(
                "   A   ",
                "  B B  ",
                " C   C ",
                "D     D",
                " C   C ",
                "  B B  ",
                "   A   "
        );

        Diamond diamond = newDiamond('D');

        assertThat(diamond.asListOfRows(), is(sameBeanAs(expectedRows)));
    }

}

The simplest code to make it pass? Hardcoded result. Now I needed some unit tests to drive the actual implementation, so I have created DiamondGenerator class that returns a list of rows and started writing a unit test for it. And here comes the main problem – how to split the entire task into a few smaller tasks, such that each could be done by separate class? In my signature I have a list of rows, so why not to split it into DiamondRowGenerator that will be generating rows and DiamondGenerator that will be joining rows together? This raised another question – how the method signature for generating single row should look like? Every row has a length so it seemed to be a mandatory parameter, but what else? The original character? The character for current row? Two positions of this character? If this had to be passed in from DiamondGenerator, DiamondGenerator would be doing too much and hence would be too difficult to write it. However, the character in current row and positions of this character can be derived from row number so I have decided to go with row number and row length.

I have written one test, made it pass, another test, made it pass, somewhere in the meantime acceptance test turned red but that was expected:

public class DiamondGeneratorTest {

    private final DiamondRowGenerator diamondRowGenerator
            = mock(DiamondRowGenerator.class);

    private final DiamondGenerator diamondGenerator
            = new DiamondGenerator(diamondRowGenerator);

    @Test
    public void createsRowsForDiamondOneByOneForA() {
        List<String> expectedRows = asList(
                "A"
        );
        when(diamondRowGenerator.generate(rowNumber(0), rowLength(1)))
                .thenReturn("A");

        List<String> actualRows = diamondGenerator.generate('A');

        assertThat(actualRows, is(sameBeanAs(expectedRows)));
    }

    @Test
    public void createsRowsForDiamondThreeByThreeForB() {
        List<String> expectedRows = asList(
                " A ",
                "B B",
                " A "
        );
        when(diamondRowGenerator.generate(rowNumber(0), rowLength(3)))
                .thenReturn(" A ");
        when(diamondRowGenerator.generate(rowNumber(1), rowLength(3)))
                .thenReturn("B B");
        when(diamondRowGenerator.generate(rowNumber(2), rowLength(3)))
                .thenReturn(" A ");

        List<String> actualRows = diamondGenerator.generate('B');

        assertThat(actualRows, is(sameBeanAs(expectedRows)));
    }

    private static int rowNumber(int rowNumber) {
        return rowNumber;
    }

    private static int rowLength(int rowLength) {
        return rowLength;
    }

}

And this is the implementation. I felt that it is small enough (and easy to understand) and further refactoring is not necessary.

public class DiamondGenerator {

    private final DiamondRowGenerator diamondRowGenerator;

    public DiamondGenerator(DiamondRowGenerator diamondRowGenerator) {
        this.diamondRowGenerator = diamondRowGenerator;
    }

    public List<String> generate(char c) {
        int size = (c - 'A') * 2 + 1;

        List<String> rows = new ArrayList<>(size);
        for (int i = 0; i < size; i++) {
            rows.add(diamondRowGenerator.generate(i, size));
        }
        return rows;
    }

}

So now was the time to implement DiamondRowGenerator. The most interesting thing was how the actual output seen in the acceptance test was changing. Before I wrote the first test, the list had seven nulls. I wrote the first test and made it pass:

@Test
public void returnsFirstRowOfDiamondOneByOne() {
    String row = diamondRowGenerator.generate(rowNumber(0), rowLength(1));

    assertThat(row , is(equalTo("A")));
}

And then by running the acceptance test I could observe the progress. Now it was seven lines of “A”, instead of seven nulls, still mismatch on every row. So I wrote second test and made it pass:

@Test
public void returnsFirstRowOfDiamondThreeByThree() {
    String row = diamondRowGenerator.generate(rowNumber(0), rowLength(3));

    assertThat(row , is(equalTo(" A ")));
}

After I run the acceptance test, I could see that the first and last rows were ok, so I was one step closer towards solution. However there was still mismatch on all other rows. So I wrote a test for another row within the top half of the diamond:

@Test
public void returnsThirdRowOfDiamondFiveByFive() {
    String row = diamondRowGenerator.generate(rowNumber(2), rowLength(5));

    assertThat(row , is(equalTo("C   C")));
}

Unfortunately after implementing this, the acceptance test started failing with IndexOutOfBoundsException. I assumed that the exception is thrown for rows from the bottom half of the diamond so I continued with unit tests for row generator. I wrote test for the line from bottom half:

@Test
public void returnsFourthRowOfDiamondFiveByFive() {
    String row = diamondRowGenerator.generate(rowNumber(3), rowLength(5));

    assertThat(row , is(equalTo(" B B ")));
}

It failed with IndexOutOfBoundsException too what gave me more confidence that my acceptance test is failing for the same reasons. After making this test pass, the acceptance test turned green which means that feature was implemented. This is how my DiamondRowGenerator looked like:

public class DiamondRowGenerator {

    public String generate(int rowNumber, int rowLength) {
        char[] array = new char[rowLength];
        Arrays.fill(array, ' ');

        int distanceFromCentre;
        if (rowNumber <= rowLength / 2) {
            distanceFromCentre = rowNumber;
        } else {
            distanceFromCentre = rowLength - 1 - rowNumber;
        }

        char character = (char) ('A' + distanceFromCentre);

        array[rowLength / 2 - distanceFromCentre] = character;
        array[rowLength / 2 + distanceFromCentre] = character;

        return new String(array);
    }

}

And that was Diamond class – data object with static constructor, which was acting as an entry point to the system:

public class Diamond {

    private static final DiamondGenerator DIAMOND_GENERATOR
            = new DiamondGenerator(new DiamondRowGenerator());

    private final List<String> rows;

    private Diamond(List<String> rows) {
        this.rows = rows;
    }

    public static Diamond newDiamond(char c) {
        return new Diamond(DIAMOND_GENERATOR.generate(c));
    }

    public List<String> asListOfRows() {
        return rows;
    }

}

Now was the time to have a final look on all the code I wrote so far and clean up all minor details. Of course for every unit test I was following the test-implement-refactor cycle too.

I was wondering whether I should refactor my code so more, extract some methods etc., but I have decided to leave it as it is. The code is relatively short and easy to understand and I did not want to end up with methods explosion. Each of them would be also taking multiple parameters and it all together could in fact decrease readability instead of increasing it.

One thing that I skipped in my solution is input correctness. I probably should add to the Diamond class an if statement checking that character is in range A to Z.

The code is available in my github repository here.

I would also like to attract your attention to sameBeanAs hamcrest matcher which I used in a few tests. It is very nice tool from shazamcrest which serialises two objects to JSON and does a string comparison between them. No getters or public fields needed, serialises nested objects too and throws ComparisonFailure rather than AssertionError (so IDE can display nice diff).

Here are some other interesting solutions and posts relating this topic:
2014-11-23 – Recycling Tests in TDD by Seb Rose
2014-11-25 – diamond-kata by Krzysztof Jelski in Python
2014-11-29 – Thinking Before Programming by Alastair Cockburn in Ruby
2014-11-29 – TDD on the Diamond Problem by Ron Jeffries in Ruby
2014-11-30 – Another Approach to the Diamond Kata by George Dinwiddie in Ruby
2014-11-30 – Diamond Kata by Sandro Mancuso in Java
2014-12-06 – Diamond Kata using Clojure and TDD by Ivan Sanchez in Clojure
2014-12-06 – diamond-problem-in-clojure by Philip Schwarz in Clojure
2014-12-09 – Diamond recycling (and painting yourself into a corner) by Seb Rose
2014-12-?? – print “squashed-circle” diamond by Jon Jagger
2015-01-10 – TDD with only Property-Based Tests by Nat Pryce in Scala

About Jaroslaw Pawlak

I have done MSci in Computer Science at King’s College London and currently work as Software Engineer specialising in Java. I spend most of my time in front of computer improving my programming (and other) skills or just relaxing with a good game. I also train some sports but at the moment I am not a member of any club. I love cycling and volleyball, but I have also played a lot of football, tennis and trained martial arts.

Posted on January 17, 2015, in Test Driven Development. Bookmark the permalink. 6 Comments.

  1. Reblogged this on Mani's fun & useful blogs and commented:
    Here’s some nifty work by Jarek, I have admired his TDD, mocking and coding practises and so share this with others.

  2. Overall its a great post, that you have mildly challenge Nat Pryce’s work on the same Kata (you know he co-author GOOS along with Steve Freeman, wink wink), is also something to take note of.

    A few things I couldn’t help notice, and you will call me different for that, but I rather be simple and explanatory than assume the reader or someone who amends my code to understand whats happening here or there:

    Use of single letter variables – if this solution grows (in this case not relevant), the ‘i’ does not explain much, same with ‘size’, rather you called them ‘rowIndex’ and ‘numberOfRows’ respectively:

    for (int i = 0; i < size; i++) {
    rows.add(diamondRowGenerator.generate(i, size));
    }

    There are a number of things, and the same things in the below blocks:

    Why are you comparing the expression in the if(…) and why is the else block doing that, would be good to have them as method calls, cause actually these methods explain your functionality better when you look at the method:

    public String generate(int rowNumber, int rowLength) {

    int distanceFromCentre;
    if (rowNumber <= rowLength / 2) {
    distanceFromCentre = rowNumber;
    } else {
    distanceFromCentre = rowLength – 1 – rowNumber;
    }
    .
    .
    .
    }

    The expressions between the […] can also be made into a method with a good name, and that way it will be clear just readin this line instead of trying to figure out whats happening here, why are we dividing and adding or subtracting:

    array[rowLength / 2 – distanceFromCentre] = character;
    array[rowLength / 2 + distanceFromCentre] = character;

    Its likely your codebase has changed and you haven't pushed the changes (I did look at the github repo as well).

    I think I ended up code-reviewing your work ;) but these things hide the core logic behind the DiamondGenerator and its supporting classes.

    But I totally like the acceptance test approach, even if you have a soluton that uses smaller functions to work, one big function calling the smaller ones and an acceptance test around it is always a boon.

    Also a question on shazamcrest (not related to this post):
    How does it handle a class that has been inherited at various levels and has a number of private variables at each hierarchy?

    Does it compare all these fields or only the public ones or one those in the direct class (and not its parent classes)

    • You are right about the changes. Although I would like to note here “Avoid mental mapping” chapter of “Clean Code” by Robert C. Martin in which he suggests to not overengineer solutions and not change common mappings – e.g. using i,j when iterating over arrays is a kind of standard so changing it actually adds complexity.

      I have made some refactoring, distanceFromCentre became a function, however it is difficult to give it a name that describes if-else statement and I wouldn’t like to extract another two methods.

      Similarly, describing this block

      array[rowLength / 2 + distanceFromCentre] = character;
      array[rowLength / 2 + distanceFromCentre] = character;
      

      is challenging, especially that it may set one or two characters. This code is quite small and is not going to change so I have left it unextracted, but if you can think of a good name (without ending with methods explosion) I am happy to change it ;)

      In regards to your shazamcrest question – it serializes all non-static fields (no matter access modifier), including all super classes, e.g. this code:

      @Test
      public void test() {
          assertThat(new Object(), sameBeanAs(new B()));
      }
      
      private static class A {
          private static final Integer i = 3;
          private final String a = "1";
      }
      
      private static class B extends A {
          private final String b = "2";
      }
      

      shows this expectation:

      {
        "b": "2",
        "a": "1"
      }
      
  3. Thanks for your quick reply and understanding where I come from.

    For the below first you are right its too simple and small implementation to refactor:

    array[rowLength / 2 + distanceFromCentre] = character;
    array[rowLength / 2 - distanceFromCentre] = character;
    

    But we could do one of the two or something else:
    Solution 1

    array[getAbsolute_Left_PositionFromCentre] = character;
    array[getAbsolute_Righ_tPositionFromCentre] = character;
    

    Solution 2 (composed methods)

    fillThe( leftPosition  , ofTheArray, withThisCharacter )
    fillThe( rightPosition, ofTheArray, withThisCharacter )
    

    (few lines are added but leaves the code in a much cleaner, understandable and also maintainable state, plus logic and other bits can be tested in isolation)

    Its late in the evening so its not exactly what I mean but more of less this way.

    Extracting methods from the logic between the brackets earlier should help especially when JaCoCo or PiTest tell you that this line is missing branch coverage, its hard to find out which branches (logical values) are being missed but when made into methods its easier to test these methods in isolation.

    Going back to the ShazamCrest question, have you tried using classes that implement the Serialize interface, and then use ShazamCrest on them – my finding has been it doesn’t fully parse such classes (Gson parser might be to blame), but if you can show it working I’ll be happy to learn how you did it.

    • Your solution 2 is an interesting seed for a large discussion. I like this style, although to me it not always seems appropriate. I think it works best with single parameter. The problem that I have faced a few times is that I had to choose between whether a method call reads well or method signature reads well (i.e. when you look on the method and try to understand what this method is doing). Also, another thing worth considering is a stack trace. Let’s say something crashed on live environment and all we see in logs is:

      java.lang.ArrayIndexOutOfBoundsException: 7
      	at com.companyname.diamond.DiamondRowGenerator.fillThe(DiamondRowGenerator.java:38)
      	at com.companyname.diamond.DiamondRowGenerator.generate(DiamondRowGenerator.java:14)
      	at com.companyname.diamond.DiamondGenerator.generate(DiamondGenerator.java:19)
      	at com.companyname.diamond.Diamond.newDiamond(Diamond.java:16)
      	at com.companyname.endpoint.DiamondEndpoint.getDiamond(DiamondEndpoint.java:24)
      

      It would be much easier to figure out the problem if method was called e.g. “setCharactersInRow”, however we will go to source anyway so maybe it’s not a large problem.

      Your point about mutation testing is interesting – I didn’t use mutation testing a lot and didn’t think about the fact that it may be giving better diagnostics. As far as I remember PIT was showing for each line that failed what was the mutation, so is it really that large advantage? We also come to the point why to use mutation testing? If the system is relatively new and test driven, there should be no need for that. And if you apply mutation testing on old legacy code with test coverage <50%, the code is probably very bad anyway and you can probably forget that you will find private methods with meaningful names. It's most likely that you will see a single method 300 lines long with variables called a, b, c.

      In regards to Shazamcrest, I did a few quick tests just now and everything was fine, do you have an example? The implemented interface should not matter for the serialization to json. However, I would not be surprised if there was another problem with Gson. One of the recent problems was that Gson was not serializing fields of type com.google.common.base.Optional. Luca Naldini has fixed it by adding custom serializer for this type (will be released probably in next version – 0.10), but I have no clue why Gson was treating this type differently. It seems to me that Gson is a bit unreliable for serialization as you cannot predict which fields it will serialize and which fields it will not serialize.

      • Okay all arguments seems to be sound. In the end its developers choice if the consensus is there we go with that flow.

        About ShazamCrest I’ll try to recreate the situation, I clearly remember it involved an Enum and parent class that implemented Serialize. Try it out and see if it works. I’ll come back with an example as well.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: