Word Chain Implementation follow up

Question

it's hard for me to implement all those features that were suggested, i did so many thing wrong.

can you please review this update on my code, concerning OOP, clean code, and maybe an eye on the algorithm used

What's note-worthy: the code looks very more clear now, single responsibility seems to be corrected, no more brain killer...

Main class

skipping the wrapping class, just posting the main method

public static void main( String[] args )
{
    WordChainBuilder wordChainBuilder = 
        new WordChainBuilder("src/main/resources/words.txt");

    Collection<String> goldChain = wordChainBuilder.build("gold", "lead");
    System.out.println("chain: "+goldChain);

    Collection<String> rubyChain = wordChainBuilder.build("ruby", "code");
    System.out.println("chain: "+rubyChain);
}

WordChainBuilder

class WordChainBuilder {

    private final WordReader wordReader;

    WordChainBuilder(String wordListFilename) {
        wordReader = new WordReader(wordListFilename);
    }

    Collection<String> build(String start, String destiny) {
        if (start == null || destiny == null || start.length() != destiny.length()) {
            throw new IllegalArgumentException();
        }
        return build(new Node(start), new Node(destiny)).asStrings();
    }

    NodeList build(Node start, Node destiny) {
        List<String> words = wordReader.readAllWordsWithLength(start.getLength());
        WordList wordList = new WordList(words);
        NodeList currentDepth = new NodeList(start);
        while (!currentDepth.isEmpty()) {
            NodeList nextDepth = new NodeList();
            for (Node node : currentDepth.getNodes()) {
                if(node.isDestiny(destiny)){
                    return buildChain(node);
                }
                NodeList candidates = findCandidates(node, wordList);
                nextDepth.addAll(candidates);
            }
            wordList.removeAll(nextDepth.asStrings());
            currentDepth = nextDepth;
        }
        return NodeList.emptyList();
    }

    private NodeList findCandidates(Node node, WordList wordList) {
        NodeList derivedNodes = new NodeList();
        for (String derivedWord : wordList.getOneLetterDifferenceWords(node.getWord())) {
            Node derivedNode = new Node(derivedWord);
            derivedNode.setPredecessor(node);
            derivedNodes.add(derivedNode);

        }
        return derivedNodes;
    }

    private NodeList buildChain(Node node) {
        NodeList chain = new NodeList();
        while (node.hasPredecssor()){
            chain.addFirst(node);
            node = node.getPredecessor();
        }
        chain.addFirst(node);
        return chain;
    }

}

Node

class Node {

    private Node predecessor;
    private final String word;

    Node(String word) {
        if (word == null || word.isEmpty()){
            throw new IllegalArgumentException();
        }
        this.word= word;
    }

    String getWord() {
        return word;
    }

    void setPredecessor(Node node){
        predecessor = node;
    }

    Node getPredecessor() {
        return predecessor;
    }

    boolean hasPredecssor(){
        return predecessor != null;
    }

    int getLength(){
        return word.length();
    }

    boolean isDestiny(Node destiny) {
        return word.equals(destiny.word);
    }

    @Override
    public String toString() {
        return word;
    }
}

i'm not sure if i did get the idea of first class collection properly...

NodeList

class NodeList extends ArrayList<Node>{

    NodeList(Node start) {
        super();
        add(start);
    }

    NodeList() {
        super();
    }

    static NodeList emptyList() {
        return new NodeList();
    }

    Collection<String> asStrings(){
        return stream().map(Node::getWord).collect(Collectors.toList());
    }

    void addFirst(Node node){
        add(0, node);
    }

}

same thing here, is that a proper implementet of a first class collection ?

WordList

class WordList extends ArrayList<String> {

    private final List<String> words;

    WordList(List<String> words){
        this.words = words;
    }

    List<String> getOneLetterDifferenceWords(String word) {
        OneLetterDifference oneWordDifference = new OneLetterDifference(word);
        return words.stream().filter(oneWordDifference::test).collect(Collectors.toList());
    }

}

OneLetterDifference

class OneLetterDifference implements Predicate<String> {

    private final String referenceWord;

    OneLetterDifference(String referenceWord){
        this.referenceWord = referenceWord;
    }

    @Override
    public boolean test(String word) {
        int difference = 0;
        for (int i = 0; i < word.length(); i++) {
            if (word.charAt(i) != referenceWord.charAt(i)) {
                difference = difference + 1;
                if (difference > 1){
                    return false;
                }
            }
        }
        return difference == 1;
    }
}

WordReader

class WordReader {

    private final String filename;

    public WordReader(String filename) {
        this.filename = filename;
    }

    List<String> readAllWordsWithLength(int length) {
        try (Stream<String> lines = Files.lines(Paths.get(filename), Charset.defaultCharset())) {
            return lines.filter(line -> line.length() == length).collect(Collectors.toList());
        } catch (IOException e) {
            e.printStackTrace();
        }
        return Collections.emptyList();
    }
}

Answer 1

Dependency Injection & Responsibility

Currently your constructor delegates the wordListFilename to the WordReader, which you build there with new.

public WordChainBuilder(String wordListFilename) {
    wordReader = new WordReader(wordListFilename);
}

The relationship between WordChainBuilder and WordReader is called composition in UML.

Imagine you want to write a Unit-Test and a Unit-Test has to be fast and has to test only one unit. A test for WordChainBuilder can't be a Unit-Test because it depends on the file system via WordReader and is therefore not fast.

Imagine WordChainBuilder should now read files from a database. We have to change the constructor.

The definition of Robert C. Martin for responsibilities is "There should never be more than one reason for a class to change"

So WordChainBuilder have still more than one responsibility: reading words and build the chain.

The responsibility should only be to build the chain. If you create an interface called WordProvider, you can easily switch from a file reader to a database reader, assume they are of typeWordProvider.

Let's change the composition to aggregation:

public WordChainBuilder(WordProvider provider) {
    this.provider = provider;
}

Now it is possible to write unit-tests, since WordChainBuilder doesn't depend directly on the file-system anymore and we could write a Mock and inject it to WordChainBuilder.

The Value Object Word

a value object is a small object that represents a simple entity whose equality is not based on identity: i.e. two value objects are equal when they have the same value

In your code base I read often word but it is from type String. Why don't you create a class for it?

The statement start.length() != destiny.length() could be written as start.hasEqualLength(destiny). The implementation OneLetterDifference of a FunctionalInterface is actually a method that belongs to the class Word, because you compare to Strings that represents a Word.