HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Solution for the BankOCR kata

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thebankocrkataforsolution

Problem

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 getAccountNumbers(File sourceFile) throws IOException {

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

for (int lineIndex = 0; lineIndex lines = new ArrayList();

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();
}

Solution

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].

Code Snippets

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

Context

StackExchange Code Review Q#128513, answer score: 4

Revisions (0)

No revisions yet.