Welcome Back
If you are here, I'm hoping you read the first post. Otherwise, this one won't make much sense.
First, I re-read my code and I noticed I made some possible mistakes or maybe sub-optimal choices. So, let's fix them now. There's no time like the present.
Actually, that's tip #1 for this post. Fix things as soon as you find them.
Let's review the method I extracted in the previous blog to find the problem.
ImageType validateImageType(String stringToMatch)
{
for (ImageType imageType : ImageType.values() )
{
if (stringToMatch.toUpperCase().endsWith(imageType.name()))
return imageType;
}
return null;
}
Now, Java developers will notice right away that I am creating a new String every time I go through the loop. It's probably not a huge deal, but if I'm going into this method a lot then it's really best to move this out of the loop.
ImageType validateImageType(String stringToMatch)
{
String upperCaseStringToMatch = stringToMatch.toUpperCase();
for (ImageType imageType : ImageType.values() )
{
if (upperCaseStringToMatch.endsWith(imageType.name()))
return imageType;
}
return null;
}
That's better. I bet my test runs faster now.
So it does. If you remember from the previous post the testInvalidImageTypeIsMatchedForBmpUrl test took .02 seconds. I like seeing that the test is faster now.
Okay, second problem. I refactored code and did not introduce a test. That's bad practice. So, that leads us to tip #2. Always refactor with tests. Okay, so strictly speaking, I am still executing this code during the original test, but that's not really the point. If I refactor, it's for a couple of reasons. First, I want to make the code more readable and therefore easy to modify. Second, I want to be able to test very specific behavior at the unit test level.
Now, it could be argued that since this is really just setting properties on an HttpClient connection that we really don't want to exercise this code as part of a unit test. Fair enough. Actually, that's why I ignored adding tests to begin with. However, I would like to make sure I can extract the connection logic away so that I can test business logic without it. But, that will take some more refactoring. For now, let's revisit the order in which we wanted to refactor.
- Validation of the art type by URL suffix.
- Image art retrieval.
- Local storage of the art.
- Updating the MediaFileStore.
Validation of the art type is okay, but it could be improved. However, I'm not going to improve it just yet. I would like to fix this method first and then make improvements. So, that's tip #3. Try not to add new features until you have finished refactoring the current behavior.
However, I would like to fix one thing that's sticking out like a sore thumb. I really don't like the fact that I have to modify the name of the enum by calling toLowerCase(). Let's fix that.
public enum ImageType
{
JPG,
GIF,
PNG;
@Override
public String toString()
{
return name().toLowerCase();
}
}
I introduced a toString() method on the enum ImageType. Now, let's call toString instead in the code.
... String suffix = imageType.toString(); ...
Cool. Now, let's look at the next refactoring.
A closer inspection of image art retrieval shows a couple of different steps going on. First, is the HTTP request that happens. Second, a file is created to store the content from the HTTP response and last is a copy of the response into the file. Note that all of this happens within a try block so that if something goes wrong with the request, the file never even gets created. However, it's easy enough to delete the file if something goes wrong and I would like to not hold the connection open while I'm busy doing other things. For one thing, we may have made the HTTP request only to find out that we are not allowed by the SecurityService to write to this file. That would be a bummer.
Therefore, it's settled. I'll first refactor file creation and then do the HTTP request. That way, I can pass the created file in as an argument and let my refactored code write to the file.
First, I'm going to add a comment into the current method where I want file creation to happen.
This gives me a skeleton for my new code. Now, I'll use copy/paste to create my new method.
Time to create a test.
You will notice how similar this test code is to the original test. Once again, I'm using Mockito to create the SecurityServiceMock and enforce stub behavior. Doing this, I could easily enforce a negative test case.
Since I am creating a file, I want to make sure this test doesn't register on the slow radar.
So far, so good.
One last change to use the refactored code. Now, I can simply uncomment the line File newCoverFile = createCoverFile(path, suffix); and remove the lines that I copied to createCoverFile.
Done. Test one more time, just to make sure. All green = all good.
Now, let's refactor image art retrieval.
Okay, I introduced an Enum if you want to be pedantic about it.
However, now I'm feeling an interface coming on. And the reason is that connection logic really doesn't belong in this class.
One thing that's clear upon reading the code is that there is really nothing special about fetching the image content. It's really just returning an InputStream containing the image. It doesn't matter that the connection is HTTP, FTP, or file based. So, I think it's time to introduce an interface.
What I want to happen is that I pass a url and the file in as parameters and that the content from the url gets written to the file. Something like the following:
This seems like something that should be in a util package. But, first I would like to copy the existing code to an implementation class.
Note that it's pretty naive code. It assumes this is an HTTP url. I didn't change anything from the original code base, but this makes it obvious that there is some room for possible improvement. Next, I'll create the interface and have the class implement it. I won't show this interface code because it's really trivial.
Now, this allows me to use dependency injection and fake out the backend. But, before I do that it's important to make sure I have working tests.
The above is not a unit test. There's just no way given that it's all about establishing a connection and getting the InputStream back. What I would like to do is to put this into the category of functional tests and only have it execute during special phases - like at the top of the pyramid. I won't show how to do this today, but just know that it's not too hard to make this happen.
I'll refactor the main code again so that the ContentRetriever is set via dependency injection and then refactor the saveCoverArt method to use the ContentRetriever to write to the output file. Here's the method refactored.
It's time to also update the original test that I created. I'll have to create a mock to hold the ContentRetriever and use it during my test. I'm not going to validate the existence of the output file anymore because my mock won't create it. Instead, I'll just use mock verifications. This should significantly speed up my tests.
Now that I'm avoiding I/O, let's check the test execution time.
Better, but not great. It's still nearly 3/4 of a second. And, that doesn't smell like a unit test.
Now, I can open the implementation of MediaFileService and move this code over.
I'll skip the boring details here, but I also noticed that isMediaFile needed to move over as well.
Here's the refactored method.
I jumped ahead a bit in the refactoring. Not only did I substitute in my new code on MediaFileService, but I also moved three lines out to a new method called persistAndBackup.
This code is now very simple to follow. I'm validating the image type, creating a file to store the cover image in and refreshing the MediaFileService.
Now, I can simplify my test even further. There's no need for MediaFileMock anymore because that's now not a required part of this method.
The code is much better, but in the end I still have a functional test. I can't do anything about it unless I want to abstract away the file handling from the interface I created and from the MediaFileService. That will entail a lot more refactoring, and I'm not sure it's warranted just to make sure this code is more maintainable. It will probably be better to stop here and separate my functional tests from my unit tests and make sure I have a comprehensive suite of unit tests in place for the next person that comes along.
Last tip: Resist API modification.
During this refactoring, I didn't modify the method name or signature. There will be times when it makes perfect sense to refactor the API, but you will risk breaking the client by doing this. So, think about extending the API rather than modifying it. You can always deprecate something over time.
A closer inspection of image art retrieval shows a couple of different steps going on. First, is the HTTP request that happens. Second, a file is created to store the content from the HTTP response and last is a copy of the response into the file. Note that all of this happens within a try block so that if something goes wrong with the request, the file never even gets created. However, it's easy enough to delete the file if something goes wrong and I would like to not hold the connection open while I'm busy doing other things. For one thing, we may have made the HTTP request only to find out that we are not allowed by the SecurityService to write to this file. That would be a bummer.
Therefore, it's settled. I'll first refactor file creation and then do the HTTP request. That way, I can pass the created file in as an argument and let my refactored code write to the file.
First, I'm going to add a comment into the current method where I want file creation to happen.
void saveCoverArt(String path, String url) throws Exception
{
ImageType imageType = validateImageType(url);
if (null == imageType)
{
LOG.info("Cover image " + url + " is not a valid image type.");
return;
}
String suffix = imageType.toString();
/* File newCoverFile = createCoverFile(path, suffix); */
...
This gives me a skeleton for my new code. Now, I'll use copy/paste to create my new method.
File createCoverFile(String path, String suffix) throws Exception
{
File newCoverFile = new File(path, "cover." + suffix);
if (!securityService.isWriteAllowed(newCoverFile)) {
throw new Exception("Permission denied: " + StringUtil.toHtml(newCoverFile.getPath()));
}
return newCoverFile;
}
Time to create a test.
@Test
public void testCreateCoverFileForCurrentPathAndSuffixJpg() throws Exception
{
when(securityServiceMock.isWriteAllowed(notNull(File.class))).thenReturn(true);
File outputFile = coverArtService.createCoverFile(SAVE_PATH, ImageType.JPG.toString());
assertNotNull(outputFile);
verify(securityServiceMock).isWriteAllowed(notNull(File.class));
}
You will notice how similar this test code is to the original test. Once again, I'm using Mockito to create the SecurityServiceMock and enforce stub behavior. Doing this, I could easily enforce a negative test case.
Since I am creating a file, I want to make sure this test doesn't register on the slow radar.
So far, so good.
One last change to use the refactored code. Now, I can simply uncomment the line File newCoverFile = createCoverFile(path, suffix); and remove the lines that I copied to createCoverFile.
Done. Test one more time, just to make sure. All green = all good.
Now, let's refactor image art retrieval.
Introducing Interfaces
Up to this point, I haven't introduced any new code. I have simply rearranged code inside of the existing class.Okay, I introduced an Enum if you want to be pedantic about it.
However, now I'm feeling an interface coming on. And the reason is that connection logic really doesn't belong in this class.
One thing that's clear upon reading the code is that there is really nothing special about fetching the image content. It's really just returning an InputStream containing the image. It doesn't matter that the connection is HTTP, FTP, or file based. So, I think it's time to introduce an interface.
What I want to happen is that I pass a url and the file in as parameters and that the content from the url gets written to the file. Something like the following:
... /* writeContentToFile( url, newCoverFile ); */ ...
This seems like something that should be in a util package. But, first I would like to copy the existing code to an implementation class.
public class HttpContentRetriever
{
HttpClient getHttpClient()
{
HttpClient client = new DefaultHttpClient();
HttpConnectionParams.setConnectionTimeout(client.getParams(), 20 * 1000);
HttpConnectionParams.setSoTimeout(client.getParams(), 20 * 1000);
return client;
}
public void writeContentToFile( String url, File newCoverFile ) throws IOException
{
/* writeContentToFile( url, newCoverFile ); */
InputStream input = null;
OutputStream output = null;
HttpClient client = getHttpClient();
try {
/* getInputStream( client ) */
HttpGet method = new HttpGet(url);
HttpResponse response = client.execute(method);
input = response.getEntity().getContent();
// Write file.
output = new FileOutputStream(newCoverFile);
IOUtils.copy(input, output);
} finally {
IOUtils.closeQuietly(input);
IOUtils.closeQuietly(output);
client.getConnectionManager().shutdown();
}
}
}
Note that it's pretty naive code. It assumes this is an HTTP url. I didn't change anything from the original code base, but this makes it obvious that there is some room for possible improvement. Next, I'll create the interface and have the class implement it. I won't show this interface code because it's really trivial.
Now, this allows me to use dependency injection and fake out the backend. But, before I do that it's important to make sure I have working tests.
public class HttpContentRetrieverTest
{
static final String DARK_SIDE_PNG_URL =
"http://upload.wikimedia.org/wikipedia/en/thumb/3/3b/Dark_Side_of_the_Moon.png/220px-Dark_Side_of_the_Moon.png";
@Before
public void setUp() {
}
@After
public void tearDown()
{
File f = new File("cover.png");
if ( f.exists() )
f.delete();
}
@Test
public void testWriteContentToFile() throws Exception
{
File newCoverFile = new File("cover.png");
HttpContentRetriever instance = new HttpContentRetriever();
instance.writeContentToFile(DARK_SIDE_PNG_URL, newCoverFile);
assertTrue(newCoverFile.exists());
}
}
The above is not a unit test. There's just no way given that it's all about establishing a connection and getting the InputStream back. What I would like to do is to put this into the category of functional tests and only have it execute during special phases - like at the top of the pyramid. I won't show how to do this today, but just know that it's not too hard to make this happen.
I'll refactor the main code again so that the ContentRetriever is set via dependency injection and then refactor the saveCoverArt method to use the ContentRetriever to write to the output file. Here's the method refactored.
void saveCoverArt(String path, String url) throws Exception
{
ImageType imageType = validateImageType(url);
if (null == imageType)
{
LOG.info("Cover image " + url + " is not a valid image type.");
return;
}
String suffix = imageType.toString();
File newCoverFile = createCoverFile(path, suffix);
contentRetriever.writeContentToFile( url, newCoverFile );
backup(newCoverFile, new File(path, "cover.backup." + suffix));
MediaFile dir = mediaFileService.getMediaFile(path);
// Refresh database.
mediaFileService.refreshMediaFile(dir);
dir = mediaFileService.getMediaFile(dir.getId());
// Rename existing cover file if new cover file is not the preferred.
try {
File coverFile = mediaFileService.getCoverArt(dir);
if (coverFile != null && !isMediaFile(coverFile)) {
if (!newCoverFile.equals(coverFile)) {
coverFile.renameTo(new File(coverFile.getCanonicalPath() + ".old"));
LOG.info("Renamed old image file " + coverFile);
// Must refresh again.
mediaFileService.refreshMediaFile(dir);
}
}
} catch (Exception x) {
LOG.warn("Failed to rename existing cover file.", x);
}
}
It's time to also update the original test that I created. I'll have to create a mock to hold the ContentRetriever and use it during my test. I'm not going to validate the existence of the output file anymore because my mock won't create it. Instead, I'll just use mock verifications. This should significantly speed up my tests.
@Test
public void testSaveCoverArtForDarkSideofTheMoonUsingPng() throws Exception
{
MediaFile mediaFileMock = mock(MediaFile.class);
when(securityServiceMock.isWriteAllowed(notNull(File.class))).thenReturn(true);
when(mediaFileServiceMock.getMediaFile(SAVE_PATH)).thenReturn(mediaFileMock);
coverArtService.saveCoverArt(SAVE_PATH, DARK_SIDE_PNG_URL);
verify(securityServiceMock).isWriteAllowed(notNull(File.class));
verify(mediaFileServiceMock).getMediaFile(SAVE_PATH);
verify(contentRetrieverMock).writeContentToFile(notNull(String.class), notNull(File.class));
}
Now that I'm avoiding I/O, let's check the test execution time.
Better, but not great. It's still nearly 3/4 of a second. And, that doesn't smell like a unit test.
Interface Segregation
Looking at the last operation that's occurring here, it seems incongruent with the rest of this class. It's really more concerned with what's happening in MediaFileService. Perhaps I can move it there.
First, I'll add a comment for what I think should happen.
/* mediaFileService.refreshCoverForPath(newCoverFile, path); */
Now, I can open the implementation of MediaFileService and move this code over.
I'll skip the boring details here, but I also noticed that isMediaFile needed to move over as well.
Here's the refactored method.
void saveCoverArt(String path, String url) throws Exception
{
ImageType imageType = validateImageType(url);
if (null == imageType)
{
LOG.info("Cover image " + url + " is not a valid image type.");
return;
}
File newCoverFile = persistAndBackup( path, url, imageType );
mediaFileService.refreshCoverForPath(newCoverFile, path);
}
I jumped ahead a bit in the refactoring. Not only did I substitute in my new code on MediaFileService, but I also moved three lines out to a new method called persistAndBackup.
This code is now very simple to follow. I'm validating the image type, creating a file to store the cover image in and refreshing the MediaFileService.
Now, I can simplify my test even further. There's no need for MediaFileMock anymore because that's now not a required part of this method.
@Test
public void testSaveCoverArtForDarkSideofTheMoonUsingPng() throws Exception
{
when(securityServiceMock.isWriteAllowed(notNull(File.class))).thenReturn(true);
coverArtService.saveCoverArt(SAVE_PATH, DARK_SIDE_PNG_URL);
verify(securityServiceMock).isWriteAllowed(notNull(File.class));
verify(contentRetrieverMock).writeContentToFile(notNull(String.class), notNull(File.class));
verify(mediaFileServiceMock).refreshCoverForPath(notNull(File.class), notNull(String.class));
}
The code is much better, but in the end I still have a functional test. I can't do anything about it unless I want to abstract away the file handling from the interface I created and from the MediaFileService. That will entail a lot more refactoring, and I'm not sure it's warranted just to make sure this code is more maintainable. It will probably be better to stop here and separate my functional tests from my unit tests and make sure I have a comprehensive suite of unit tests in place for the next person that comes along.
Last tip: Resist API modification.
During this refactoring, I didn't modify the method name or signature. There will be times when it makes perfect sense to refactor the API, but you will risk breaking the client by doing this. So, think about extending the API rather than modifying it. You can always deprecate something over time.
