4
\$\begingroup\$

Next Saturday I'd like to conduct a TDD demo in our local developer meetup. For this purpose I want to implement the first user story of the BankOCR kata from the Coding Dojo Wiki.

Here is the solution I came up with. Any feedback is appreciated.

AccountNumberParser.java

package com.demo.bankorc;

import java.io.BufferedReader;
import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.LinkedList;
import java.util.List;


public class AccountNumberParser {

    private static final int NUMBER_OF_DIGIT_COLS = 3;
    private static final int NUMBER_DIGIT_ROWS = 3;
    private static final int NUMBER_OF_DIGITS = 9;

    public static List<String> getAccountNumbers(File sourceFile) throws IOException {

        List<String> accountNumbers = new LinkedList<String>();
        String[] fileContents = readLines(sourceFile);

        for (int lineIndex = 0; lineIndex < fileContents.length; lineIndex += 4) {
            String[] accountEntry = new String[3];
            accountEntry[0] = fileContents[lineIndex];
            accountEntry[1] = fileContents[lineIndex + 1];
            accountEntry[2] = fileContents[lineIndex + 2];
            accountNumbers.add(parseEntry(accountEntry));
        }

        return accountNumbers;
    }

    private static String[] readLines(File file) throws IOException {

        ArrayList<String> lines = new ArrayList<String>();

        try (BufferedReader reader = Files.newBufferedReader(file.toPath())) {            
            String line = null;
            while ((line = reader.readLine()) != null) {
                lines.add(line);
            }
        }

        return lines.toArray(new String[]{});
    }

    private static String parseEntry(String[] accountEntry) {

        StringBuilder sb = new StringBuilder();

        for (int digitIndex = 0; digitIndex < NUMBER_OF_DIGITS; digitIndex++) {
            String[] digitRows = new String[NUMBER_DIGIT_ROWS];

            int substringStartIndex = digitIndex * NUMBER_OF_DIGIT_COLS;
            digitRows[0] = accountEntry[0].substring(substringStartIndex, substringStartIndex + 3);
            digitRows[1] = accountEntry[1].substring(substringStartIndex, substringStartIndex + 3);
            digitRows[2] = accountEntry[2].substring(substringStartIndex, substringStartIndex + 3);                

            sb.append(parseDigit(digitRows));
        }

        return sb.toString();
    }

    private static String parseDigit(String[] digitRows) {

        if (Arrays.equals(digitRows, Digits.ZERO)) {
            return "0";
        } else if (Arrays.equals(digitRows, Digits.ONE)) {
            return "1";
        }  else if (Arrays.equals(digitRows, Digits.TWO)) {
            return "2";
        }  else if (Arrays.equals(digitRows, Digits.THREE)) {
            return "3";
        }  else if (Arrays.equals(digitRows, Digits.FOUR)) {
            return "4";
        }  else if (Arrays.equals(digitRows, Digits.FIVE)) {
            return "5";
        }  else if (Arrays.equals(digitRows, Digits.SIX)) {
            return "6";
        }  else if (Arrays.equals(digitRows, Digits.SEVEN)) {
            return "7";
        }  else if (Arrays.equals(digitRows, Digits.EIGHT)) {
            return "8";
        }  else if (Arrays.equals(digitRows, Digits.NINE)) {
            return "9";
        } else {
            throw new IllegalArgumentException("Cannot parse digit " + Arrays.toString(digitRows));
        }
    }

    private static class Digits {

        final static String[] ZERO = new String[] {
                " _ ",
                "| |",
                "|_|"
        };

        final static String[] ONE = new String[] {
                "   ",
                "  |",
                "  |"
        };

        final static String[] TWO = new String[] {
                " _ ",
                " _|",
                "|_ "
        };

        final static String[] THREE = new String[] {
                " _ ",
                " _|",
                " _|"
        };

        final static String[] FOUR = new String[] {
                "   ",
                "|_|",
                "  |"
        };

        final static String[] FIVE = new String[] {
                " _ ",
                "|_ ",
                " _|"
        };

        final static String[] SIX = new String[] {
                " _ ",
                "|_ ",
                "|_|"
        };

        final static String[] SEVEN = new String[] {
                " _ ",
                "  |",
                "  |"
        };

        final static String[] EIGHT = new String[] {
                " _ ",
                "|_|",
                "|_|"
        };

        final static String[] NINE = new String[] {
                " _ ",
                "|_|",
                " _|"
        };
    }
}

AccountNumberParserTest.java

package com.demo.bankorc;

import static org.junit.Assert.assertEquals;

import java.io.File;
import java.io.IOException;
import java.net.URL;
import java.util.List;

import org.junit.Test;

import com.demo.bankorc.AccountNumberParser;


public class AccountNumberParserTest {

    @Test
    public void testParseFile_SingleAccount() throws IOException {

        File testFile = getTestFile("account-123456789.txt");
        List<String> accountNumbers = AccountNumberParser.getAccountNumbers(testFile);
        assertEquals("Could not parse account number", "123456789", accountNumbers.get(0));

        testFile = getTestFile("account-490067715.txt");
        accountNumbers = AccountNumberParser.getAccountNumbers(testFile);
        assertEquals("Could not parse account number", "490067715", accountNumbers.get(0));
    }

    @Test
    public void testParseFile_MultipleAccounts() throws IOException {

        File testFile = getTestFile("multiple-accounts.txt");
        List<String> accountNumbers = AccountNumberParser.getAccountNumbers(testFile);

        assertEquals("Could not parse all account numbers", 10, accountNumbers.size());              
    }


    private File getTestFile(String fileName) {

        URL testFileUrl = ClassLoader.getSystemResource(fileName);
        return new File(testFileUrl.getPath());
    }
}

account-123456789.txt

    _  _     _  _  _  _  _ 
  | _| _||_||_ |_   ||_||_|
  ||_  _|  | _||_|  ||_| _| 

account-490067715.txt

    _  _  _  _  _  _     _ 
|_||_|| || ||_   |  |  ||_ 
  | _||_||_||_|  |  |  | _|

multiple-accounts.txt

 _  _  _  _  _  _  _  _  _ 
| || || || || || || || || |
|_||_||_||_||_||_||_||_||_|


  |  |  |  |  |  |  |  |  |
  |  |  |  |  |  |  |  |  |

 _  _  _  _  _  _  _  _  _ 
 _| _| _| _| _| _| _| _| _|
|_ |_ |_ |_ |_ |_ |_ |_ |_ 

 _  _  _  _  _  _  _  _  _ 
 _| _| _| _| _| _| _| _| _|
 _| _| _| _| _| _| _| _| _|


|_||_||_||_||_||_||_||_||_|
  |  |  |  |  |  |  |  |  |

 _  _  _  _  _  _  _  _  _ 
|_ |_ |_ |_ |_ |_ |_ |_ |_ 
 _| _| _| _| _| _| _| _| _|

 _  _  _  _  _  _  _  _  _ 
|_ |_ |_ |_ |_ |_ |_ |_ |_ 
|_||_||_||_||_||_||_||_||_|

 _  _  _  _  _  _  _  _  _ 
  |  |  |  |  |  |  |  |  |
  |  |  |  |  |  |  |  |  |

 _  _  _  _  _  _  _  _  _ 
|_||_||_||_||_||_||_||_||_|
|_||_||_||_||_||_||_||_||_|

 _  _  _  _  _  _  _  _  _ 
|_||_||_||_||_||_||_||_||_|
 _| _| _| _| _| _| _| _| _|

During the development I declared the "private" methods as "protected" in order to be able to proceed step by step. Later on I deleted the tests for the "protected" methods and changed the visibility.

I know that some people say that unit tests should not access the file system. I have done it deliberately because I think the runtime of the tests is insignificant.

You can read the source code of my solution on GitHub if you like.

\$\endgroup\$
4
\$\begingroup\$

Besides a few stylistic nuances, such as unnecessary right-hand side type parameters and initializing a String to null, my only critique is that you should use your constants NUMBER_OF_DIGIT_COLS, NUMBER_OF_DIGIT_ROWS, and NUMBER_OF_DIGITS whenever applicable, both for readability and extensibility. Take, for example,

for (int lineIndex = 0; lineIndex < fileContents.length; lineIndex += 4) {
    String[] accountEntry = new String[3];
    accountEntry[0] = fileContents[lineIndex];
    accountEntry[1] = fileContents[lineIndex + 1];
    accountEntry[2] = fileContents[lineIndex + 2];
    accountNumbers.add(parseEntry(accountEntry));
}

Now look at the following:

for (int lineIndex = 0; lineIndex < fileContents.length; lineIndex += NUMBER_OF_DIGIT_ROWS + 1) {
    String[] accountEntry = Arrays.copyOfRange(
        fileContents, lineIndex, lineIndex + NUMBER_OF_DIGIT_ROWS);
    accountNumbers.add(parseEntry(accountEntry));
}

Also, why not go ahead and test for the correct account numbers in testParseFile_MultipleAccounts()?

Ignoring whitespace fixes, I pulled my changes on GitHub [mirror].

\$\endgroup\$
  • \$\begingroup\$ My reasoning for not testing the correct account numbers in testParseFile_MultipleAccounts() was that this is already done in the previous test function. I have merged you pull request on GitHub. Thank you! \$\endgroup\$ – Janux May 17 '16 at 6:06
1
\$\begingroup\$

This is in part going to be a critique of the process you've described, this is obvious subjective so feel free to ignore the bits you don't agree with.

I had a look at your git repository and was a little disappointed to only see a single commit with the code you've posted here. Whilst from a code review perspective the current state of the code is most important, from a teaching TDD perspective the process and commits are at least as important.

The process you've described is that you've started off with protected methods, so that you could write tests, then when you were happy, made the methods private and removed the tests. This is really undermining one of the goals of TDD which is to produce testable code documented through tests.

I would tend to avoid using files from code that I am unit testing. That said, acceptance tests are an important, and often overlooked, aspect of TDD. These should be invoking the code in a way that results in it using files.

Test Code

As I've said, I would view your tests as acceptance tests since the code being tested accesses actual test files.

testParseFile_SingleAccount

This test checks two separate files for two separate account numbers. I'd consider this a bad test structure. You should be testing one input per test. I would either split this into two separate tests, or look into writing a parametrised test to allow different inputs and expectations for the same test code. I use TestCases for this in c#, under java this question would be a good starting point.

testParseFile_MultipleAccounts

This test only checks that the right number of accounts have been populated. Why doesn't it check that the right accounts have been read? The first account number extracted could be returned for every entry in the file and all of your current tests would pass.

Your test names don't really describe what's been tested. Consider something more like:

test<MethodName>_<Scenario>_<Expectation>
testGetAccountNumbers_MultipleAccounts_ReturnsCorrectNumberOfAccounts
testGetAccountNumbers_SingleAccount_ReturnsCorrectAccountNumber

You're missing test coverage. This is clear because I could introduce the bug described above whilst your tests would still pass and also because you have code that you're not currently testing (you don't test any malformed files):

else {
    throw new IllegalArgumentException("Cannot parse digit " + Arrays.toString(digitRows));
}

Actual Code

As a result of the process you've followed, I don't think you've ended up with code as testable as it could be.

Without going through the process, I can't know for sure but my general feeling is that if I went through my normal cycle, I'd have ended up with something a bit more like this:

The AccountNumberParser name is ok, however it suggests that the class only parses a single account number. I'd take it at its name and make that the classes sole responsibility and create an interface for it so that a test double can be used for unit testing. I would rename the parseEntry method to parseAccountNumber and make it public. For the moment, I'd leave the digit parsing code under the class as well, although that might change with subsequent user stories from the kata. This gives you public access to the code which will make it much easier to unit test the account parsing logic without needing to put it into files and encourage a test to document what happens in a malformed account number.

I'd put the readLines method into a LineProvider class and again, hide it behind an interface so that a test double could be used for testing. I wouldn't unit test the LineProvider class, I would cover it purely by acceptance tests.

I'd create an AccountEntriesProcessor which has interfaces for AccountNumberParser and a LineProvider passed to its constructor and uses them to implement your current getAccountNumbers method. Again this decoupling will help validate logic like the breaking up of the provided lines into the correct chunks to be passed to the parser, what happens if the parser throws an exception etc.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your tips! At the meetup I demonstrated a reimplementation where I tried to consider your advice, e.g. I have individual commits for the Red-Green-Refactor cycle: github.com/jenadevs/KataBankOCR \$\endgroup\$ – Janux Jun 11 '16 at 13:02
1
\$\begingroup\$

List<String> vs String[]

readLines(File) returns a String[], but that array is only used in a simple for-loop, which I think a List<String> will work equally well too. The reason for suggesting the latter is that you can rely on Files.readAllLines(Path) to do it in a single step for you, which eliminates the need to have your own little method.

public static List<String> getAccountNumbers(File file) throws IOException {
    List<String> result = new ArrayList<>(); // no need for doubly-linked LinkedList
    List<String> content = Files.readAllLines(file.toPath());
    // modified from @Tyler Flynn's answer
    for (int i = 0; i < inputs.size(); i += NUMBER_OF_DIGIT_ROWS + 1) {
        List<String> line = content.subList(i, i + NUMBER_OF_DIGIT_ROWS);
        // simply converting back to String[] here,
        // but you can consider passing a List to the parseEntry method
        accountNumbers.add(parseEntry(line.toArray(new String[NUMBER_OF_DIGIT_ROWS])));
    }
    return result;
}

List<String> vs char[][]

I also wonder if the comparison might be a bit more trivial if you convert the file content into a 2D char[][] array. This allows you to navigate the 2D array by 'blocks', which may make for slightly better usage of the constant fields you have at the moment.

String vs Integer output

Since you are expected to produce numbers, I wonder if it's better to create Integer (or some Number-based) values, instead of Strings. Of course, if account numbers start coming in with non-numeric characters, then as the vendor of the Award-Winning OCR enterprise system, you can start charging for these new features. :p

Testing

While it's expected that your inputs are in the form of a File, you should make it such that the actual logic can be tested easily against a List<String> or String[] input. Alternatively, you can consider something like Jimfs so that you can easily create files within an in-memory filesystem, good for testing purpose.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you for your tips! However, I think using Integer is not possible as the account number can start with a leading zero. \$\endgroup\$ – Janux Jun 11 '16 at 13:04
  • \$\begingroup\$ As the vendor of the Award-Winning OCR enterprise system, you can start charging for these new features. :p \$\endgroup\$ – h.j.k. Jun 12 '16 at 2:50

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.