Example Spaghetti Design
The following code is a REST-like service that fetches a list of files from the Amazon’s Simple Storage Service (S3) and displays them as a list of links to the contents of the files:
public class S3FilesResource {
AmazonS3Client amazonS3Client;
...
@Path('files')
public String listS3Files() {
StringBuilder html = new StringBuilder('<html><body>');
List<S3ObjectSummary> files = this.amazonS3Client.listObjects('myBucket').getObjectSummaries();
for (S3ObjectSummary file : files) {
String filePath = file.getKey();
if (!filePath.endsWith('')) { exclude directories
html.append('<a href='content?fileName=').append(filePath).append(''>').append(filePath)
.append('<br>');
}
}
return html.append('<body><html>').toString();
}
@Path('content')
public String getContent(@QueryParam('fileName') String fileName) {
throw new UnsupportedOperationException('Not implemented yet');
}
}
Why is the code difficult to test?
- There is no seam that would enable us to bypass the external dependency on S3 and thus we cannot influence what data is passed to the method and cannot test it easily with different values. Moreover we depend on network connection and correct state in the S3 service to be able to run the code.
- It’s difficult to sense the result of the method because it mixes the data with their presentation. It would be much easier to have direct access to the data to verify that directories are excluded and that the expected file names are displayed. Moreover the core logic is much less likely to change than the HTML presentation but changing the presentation will break our tests even though the logic won’t change.
What can we do to improve it?
We first test the code as-is to be sure that our refactoring doesn’t break anything (the test will be brittle and ugly but it is just temporary), refactor it to break the external dependency and split the data and presentation, and finally re-write the tests.
We start by writing a simple test:
public class S3FilesResourceTest {
@Test
public void listFilesButNotDirectoriesAsHtml() throws Exception {
S3FilesResource resource = new S3FilesResource(* pass AWS credentials ... *);
String html = resource.listS3Files();
assertThat(html)
.contains('<a href='content?fileName=dirfile1.txt'>dirfile1.txt')
.contains('<a href='content?fileName=diranother.txt'>diranother.txt')
.doesNotContain('dir'); directories should be excluded
assertThat(html.split(quote(''))).hasSize(2 + 1); two links only
}
}
Refactoring the Design
This is the refactored design, where I have decoupled the code from S3 by introducing a Facade/Adapter and split the data processing and rendering:
public interface S3Facade {
List<S3File> listObjects(String bucketName);
}
public class S3FacadeImpl implements S3Facade {
AmazonS3Client amazonS3Client;
@Override
public List<S3File> listObjects(String bucketName) {
List<S3File> result = new ArrayList<S3File>();
List<S3ObjectSummary> files = this.amazonS3Client.listObjects(bucketName).getObjectSummaries();
for (S3ObjectSummary file : files) {
result.add(new S3File(file.getKey(), file.getKey())); later we can use st. else for the display name
}
return result;
}
}
public class S3File {
public final String displayName;
public final String path;
public S3File(String displayName, String path) {
this.displayName = displayName;
this.path = path;
}
}
public class S3FilesResource {
S3Facade amazonS3Client = new S3FacadeImpl();
...
@Path('files')
public String listS3Files() {
StringBuilder html = new StringBuilder('<html><body>');
List<S3File> files = fetchS3Files();
for (S3File file : files) {
html.append('<a href='content?fileName=').append(file.path).append(''>').append(file.displayName)
.append('<br>');
}
return html.append('<body><html>').toString();
}
List<S3File> fetchS3Files() {
List<S3File> files = this.amazonS3Client.listObjects('myBucket');
List<S3File> result = new ArrayList<S3File>(files.size());
for (S3File file : files) {
if (!file.path.endsWith('')) {
result.add(file);
}
}
return result;
}
@Path('content')
public String getContent(@QueryParam('fileName') String fileName) {
throw new UnsupportedOperationException('Not implemented yet');
}
}
In practice I’d consider using the built-in conversion capabilities of Jersey (with a custom MessageBodyWriter for HTML) and returning List<S3File> from listS3Files.
This is what the test looks like now:
public class S3FilesResourceTest {
private static class FakeS3Facade implements S3Facade {
List<S3File> fileList;
public List<S3File> listObjects(String bucketName) {
return fileList;
}
}
private S3FilesResource resource;
private FakeS3Facade fakeS3;
@Before
public void setUp() throws Exception {
fakeS3 = new FakeS3Facade();
resource = new S3FilesResource();
resource.amazonS3Client = fakeS3;
}
@Test
public void excludeDirectories() throws Exception {
S3File s3File = new S3File('file', 'file.xx');
fakeS3.fileList = asList(new S3File('dir', 'mydir'), s3File);
assertThat(resource.fetchS3Files())
.hasSize(1)
.contains(s3File);
}
** Simplest possible test of listS3Files *
@Test
public void renderToHtml() throws Exception {
fakeS3.fileList = asList(new S3File('file', 'file.xx'));
assertThat(resource.listS3Files())
.contains('file.xx');
}
}Next I’d implement an integration test for the REST service but still using the FakeS3Facade to verify that the service works and is reachable at the expected URL and that the link to a file content works as well. I would also write an integration test for the real S3 client (through S3FilesResource but without running it on a server) that would be executed only on-demand to verify that our S3 credentials are correct and that we can reach S3. (I don’t want to execute it regularly as depending on an external service is slow and brittle.)
Disclaimer: The service above isn’t a good example of proper REST usage and I have taken couple of shortucts that do not represent good code for the sake of brevity.
Happy coding and don’t forget to share!
Reference: Help, My Code Isn’t Testable! Do I Need to Fix the Design? from our JCG partner Jakub Holy at the The Holy Java blog.
