3
\$\begingroup\$

I'm working on a coding challenge that tasks me with taking a file with a dictionary, reading from it, and then replacing the matching words from the input file with what is found in the dictionary.

I have a few concerns with my code.

  1. I don't want to overengineer a task that seems simple, but I also don't want to trivialize the complexity of certain I/O operations or make trivial tests for it. As a result I've created one data class that processes the dictionary from the input file and two service classes that implement two interfaces. See image below.

enter image description here

  1. That being said I have concerns about my business logic class, FileContentReplacementService, because the code seems messy and I feel like there are some things that could be improved but I just can't put my fingers on them at this very moment. Below is the full class:

FileContentReplacementService

import java.io.*;
import java.util.Map;

import Challenge2.model.FileBasedDictionary;
import org.apache.commons.io.LineIterator;
import org.apache.commons.lang3.exception.ExceptionUtils;
import org.apache.log4j.LogManager;
import org.apache.log4j.Logger;

public class FileContentReplacementService implements ContentReplacementService {

    private static final Logger LOGGER = LogManager.getLogger(FileContentReplacementService.class);
    private final RetrieverService retrieverService;
    private final FileBasedDictionary fileBasedDictionary;
    private final String outputFileName;

    public FileContentReplacementService(RetrieverService retrieverService,
                                         FileBasedDictionary fileBasedDictionary,
                                         String outputFileName) {
        this.retrieverService = retrieverService;
        this.fileBasedDictionary = fileBasedDictionary;
        this.outputFileName = outputFileName;
    }

    public boolean areMatchingWordsReplace(){
        String fileName = "src/main/resources/" + outputFileName;
        try {
            applyDictionaryToFile(fileBasedDictionary, retrieverService.getBufferedReader(), fileName);
        } catch (IOException ioe) {
            LOGGER.error(String.format("File not reachable \n %s", ExceptionUtils.getStackTrace(ioe)));
            return false;
        }

        return true;
    }

    private void applyDictionaryToFile(FileBasedDictionary fileBasedDictionary,
                                       BufferedReader bufferedReader,
                                       String fileName) throws IOException
    {
        FileOutputStream fileOutputStream = new FileOutputStream(fileName);
        BufferedOutputStream bufferedOutputStream = new BufferedOutputStream(fileOutputStream);
        LineIterator lineIterator = new LineIterator(bufferedReader);
        while (lineIterator.hasNext()) {
            String line = lineIterator.nextLine();
            bufferedOutputStream.write(replaceWords(line, fileBasedDictionary.getDictionary()));
        }
    }

    private byte[] replaceWords(String line, Map<String, String> dictionary) {
        if (line.isEmpty()) {
            return "\n\n".getBytes();
        }

        for (Map.Entry<String, String> entry : dictionary.entrySet()) {
            line = line.replaceAll(entry.getKey(), entry.getValue());
        }

        return line.getBytes();
    }
}
  1. Is there any way to trigger the exception from the try/catch block of FileContentReplacementService? I've made a few tries but I could only trigger it by closing the BufferedReader and then flushing it, but I can only do that inside the method. My test, see below, is currently not covering that bit and I would like to know if there's something that can be done to cover it.

FileContentReplacementServiceTest

import Challenge2.model.FileBasedDictionary;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnitRunner;

import java.io.*;
import java.util.Map;

import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.*;

@RunWith(MockitoJUnitRunner.class)
public class FileContentReplacementServiceTest {
    @Mock
    FileBasedDictionary fileBasedDictionary;
    @Mock
    FileRetrieverService fileRetrieverService;
    @Mock
    BufferedReader bufferedReader;

    FileContentReplacementService fileContentReplacementService;

    @Test
    public void givenValidFile_WhenWordsAreReplaced_ThenReplaceMatchingWordsIsCalled() throws IOException {
        when(fileRetrieverService.getBufferedReader()).thenReturn(bufferedReader);
        when(bufferedReader.readLine()).thenReturn("Test2", "", null);
        when(fileBasedDictionary.getDictionary()).thenReturn(Map.of("Test1", "Test2"));
        fileContentReplacementService = new FileContentReplacementService(fileRetrieverService, fileBasedDictionary, "\\testOutput\\test.txt");
        assertTrue(fileContentReplacementService.areMatchingWordsReplace());
    }
}
  1. My FileRetrieverService is basically untesteable and, despite my best efforts, I just can't find a way to be able to test it and maintain decent code quality and/or not break a number of SOLID principles.

FileRetrieverService

import java.io.BufferedReader;
import java.io.InputStreamReader;
import java.util.Objects;

public class FileRetrieverService implements RetrieverService {

    private final String fileName;

    public FileRetrieverService(String fileName) {
        this.fileName = fileName;
    }

    @Override
    public BufferedReader getBufferedReader(){
        return new BufferedReader(new InputStreamReader(Objects.requireNonNull(
                getClass().
                getClassLoader().
                getResourceAsStream(fileName))));
    }
}

FileRetrieverServiceTest

import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnitRunner;

import java.io.BufferedReader;

import static org.junit.Assert.assertNotNull;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.when;

@RunWith(MockitoJUnitRunner.class)
public class FileRetrieverServiceTest{
    @Mock
    BufferedReader bufferedReader;
    @Mock
    FileRetrieverService fileRetrieverService;

    @Test
    public void givenFileName_WhenReturnBufferedReader_ThenReturnReader(){
        when(fileRetrieverService.getBufferedReader()).thenReturn(bufferedReader);
        assertNotNull(fileRetrieverService.getBufferedReader());
    }
    @Test(expected = NullPointerException.class)
    public void givenInvalidFileName_ThenFailFast(){
        doThrow(NullPointerException.class).when(fileRetrieverService).getBufferedReader();
        fileRetrieverService.getBufferedReader();
    }
}

I would really appreciate it if someone could take a look at the code above and give me a few pointers as to what improvements could be done to make the code more easily testable and better designed.

\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

I don't think you need to test everything around opening the input file, closing it, handling exceptions, etc. For the purpose of this task, I believe you want to test your logic, and trust that FileReader or FileInputStream etc just work.

I would have a class that that can read the format of the dictionary (not provided, presumably this is FileBasedDictionary), and create a test of that class that it produces the expected Map<String, String>.

Then have a class that does business logic (i.e. the word replacement) that can operate independant of the source of the data (by taking in a flexible type like a BufferedReader). You can add a test case that demonstrates how to create a valid data source, while also demonstrating the business logic - tests are as much about documentation/demonstration as they are about checking.

import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.IOException;
import java.util.Map;

public class DictionaryReplacementService {
    
    private Map<String, String> dictionary;
    
    public DictionaryReplacementService(Map<String, String> dictionary) {
        this.dictionary = dictionary;
    }

    public void replaceWords(BufferedReader input, BufferedWriter output)
            throws IOException {
        String line;
        while ((line = input.readLine()) != null) {
            for (Map.Entry<String, String> entry : dictionary.entrySet()) {
                line = line.replaceAll(entry.getKey(), entry.getValue());
            }
            output.write(line);
        }
    }
}
import static org.junit.jupiter.api.Assertions.assertEquals;

import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.IOException;
import java.io.StringReader;
import java.io.StringWriter;
import java.util.LinkedHashMap;
import java.util.Map;

import org.junit.jupiter.api.Test;

class DictionaryReplacementServiceTest {

    @Test
    void test() throws IOException {
        String source = "It was the best of times, it was the blurst of times.";

        Map<String, String> dictionary = new LinkedHashMap<String, String>();
        dictionary.put("blurst", "worst");

        StringWriter outString = new StringWriter();
        try (BufferedReader in = new BufferedReader(new StringReader(source));
                BufferedWriter out = new BufferedWriter(outString)) {
            new DictionaryReplacementService(dictionary).replaceWords(in, out);
        }

        assertEquals("It was the best of times, it was the worst of times.",
                outString.toString());
    }

}

In this case, the user of your class would just need to replace StringReader with FileReader if that's what they wanted - the DictionaryReplacementService is not interested in that.

A last note is about exceptions and return values. Instead of returning true for success and false for failure, I prefer to return void for success and just throw any exceptions that mean the success case is not possible (in my case, the try is just to ensure the resources are closed - it doesn't catch any exceptions, and throwing an exception from a JUnit test means a test failure, which is what we want).

You said you don't want to trivialize file i/o and you want to test what happens when problems happen, but that's not a concern of DictionaryReplacementService - it only needs to signal that something went wrong. In a full program, you would have logic in the UI layer to try and recover (let the user know there was a problem, prompt them to try again or use a different file etc). You're not writing that UI layer now, so I wouldn't expect to see tests about it now.

\$\endgroup\$
1
\$\begingroup\$

@Jeremy Hunt's great answer has covered an approach that will make your classes more testable. I'm just going to add a few other things to think about, with your existing code.

given..when..then

You're naming your tests using a given..when..then approach. If that's your preference, then you may want to consider using Mockito's BDD extensions which might make your test names and their contents easier to link up. The usage is exactly the same, but the commands are aliased, so you end up with:

import static org.mockito.BDDMockito.given;

// ...

@Test
public void givenValidFile_WhenWordsAreReplaced_ThenReplaceMatchingWordsIsCalled() throws IOException {
    given(fileRetrieverService.getBufferedReader()).willReturn(bufferedReader);
    given(bufferedReader.readLine()).willReturn("Test2", "", null);
    given(fileBasedDictionary.getDictionary()).willReturn(Map.of("Test1", "Test2"));

    fileContentReplacementService = new FileContentReplacementService(fileRetrieverService, fileBasedDictionary, "\\testOutput\\test.txt");
    var matchingWordsWereReplaced = fileContentReplacementService.areMatchingWordsReplace();

    assertTrue(matchingWordsWereReplaced);
}

IsCalled

When I see a test that's got 'IsCalled' in its title, it sets my expectation that the test is going to verify that a particular call has taken place. That isn't what your test is doing, so it took me a bit by suprise.

Spacing

Tests typically consist of three parts.. Arrange/Given, Act/When, Assert/Then. Consider putting a blank line between the three sections. Having it all as one block makes it increasing difficult to identify the important bits when a test fails.

Levels of abstraction

When writing methods, maintaining the same level of abstraction can make the methods easier to understand/change/reuse. Your replaceWords method takes in a String, a Map<String, String>, but it returns a byte[].

private byte[] replaceWords(String line, Map<String, String> dictionary)

Consider having it return a String instead and leaving the responsibility of knowing about converting it to a byte[] to the calling bit of code that needs to know how to write to a file.

NOP tests

You may have added these tests, just to give something to review, however they're a great example of a common pitfall that people can fall into when starting to use mocks.

@Test
public void givenFileName_WhenReturnBufferedReader_ThenReturnReader(){
    when(fileRetrieverService.getBufferedReader()).thenReturn(bufferedReader);
    assertNotNull(fileRetrieverService.getBufferedReader());
}

You're setting up a mock to do something, then testing that it does what you've told it to do. You're not testing any of your code at all, what you're actually testing is that the mocking framework works as expected.

An alternative, since your class appears to work with file resources, could be to construct the FileRetrieverService with a known file resource and then verify that the returned buffer worked.

Class state

For me, filename doesn't feel like it's a property of the FileRetrieverService, instead it feels like it should be an argument to the getBufferedReader method. It may make sense if you're going to be calling getBufferedReader multiple times, but for me, if you're only using a field in one method, it seems like it belongs as a parameter to that method.

Testing exceptions

@Jeremy Hunt has already addressed this, but generally if something feels hard to test, it's usually a hint that your logic needs to be broken up differently. That said, to test an exception I'll usually look at the dependencies that are being injected into the system under test to see if there's a way to generate the exception via one of them. This is a bit more complicated for your example because you're after and IOException is a checked exception so needs to be declared on method signatures. Looking through the dependencies, one approach that seemed like it might work would be to have bufferedReader.readLine, throw an IOException

when(bufferedReader.readLine()).thenThrow(new IOException()); 

However this doesn't work, because LineReader translates the IOException into a RuntimeException (IllegalStateException), so you don't catch it. This lends some support to Jeremy's suggestion that it's up to the caller to deal with the Exceptions, however if you did want to catch them in the class, then should you be handling errors from the Reader or are you only really interested in writer exceptions.

If you did want to test the writer, then the next step might be to introduce a FileWriterService. This seems like a similar level of abstraction to the FileReaderService, so could be a valid object. This would allow you to manipulate the constructed output stream and throw exceptions etc. So you would end up with a test something like this:

@Mock
private BufferedOutputStream bufferedOutputStream;
@Mock
private FileWriterService fileWriterService;


@Test
public void givenValidFile_WhenFileIOError_ThenNotReplaced() throws IOException {
    when(fileRetrieverService.getBufferedReader()).thenReturn(bufferedReader);
    when(bufferedReader.readLine()).thenReturn("Test2", "", null);
    when(fileWriterService.getBufferedWriter(any())).thenReturn(bufferedOutputStream);
    when(fileBasedDictionary.getDictionary()).thenReturn(Map.of("Test1", "Test2"));

    doThrow(new IOException()).when(bufferedOutputStream).write(any());

    fileContentReplacementService = new FileContentReplacementService(fileRetrieverService, fileBasedDictionary, "\\testOutput\\test.txt", fileWriterService);

    assertFalse(fileContentReplacementService.areMatchingWordsReplace());
}

With the implementation pulled out from the FileContentReplacementService into:

public class FileWriterService {
    public BufferedOutputStream getBufferedWriter(String fileName) throws FileNotFoundException {
        FileOutputStream fileOutputStream = new FileOutputStream(fileName);
        return new BufferedOutputStream(fileOutputStream);
    }
}

Again... this might not be the best solution for your current problem, but is the general approach I'd follow for pulling out dependencies in order to facilitate testing of awkward scenarios.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.