Keep it Simple Stupid

It's one of the XP core values - simplicity.  Yet, we value the highly complex and complicated.  In enterprise environments, if we aren't working on multiple projects, hanging tough through adversity, fixing bugs, driving features in until the bitter end then we must not be working hard enough.

So, I'm here to dispel the myth that working effectively requires complexity.  In fact, I would say just the opposite is true.  Working effectively requires simplicity. 

Research in cognitive load theory (CLT) shows that humans have effective use of at most seven elements, but we use only two to four elements and that our short term memory effectively retains items for only twenty seconds unless there is something that refreshes the memory.   In software development, particularly in the enterprise, this can affect us in multiple ways.

Working on Multiple Projects

Juggling multiple projects can reduce a developer's effectiveness to a great degree, particularly if there are significant differences between projects.  Presumably, each project requires a different branch of code and may contain classes and files that have changed significantly from one release to another.  The developer must keep context of the desired change as well as the existing context of the overall codebase into account.  This is exacerbated when this is created by differing customer desires for the same feature.  

Project managers will also wrestle with the priorities across these projects which will cause the developer to go back and forth between the codebases, each time requiring a cognitive reset in order to recall from long-term memory the context of the codebase.

What is the remedy?

There may be several possibilities to consider here.  First, if there really is a need to keep separate codebases for different clients, consider how this could be done by keeping a team of developers dedicated to that client.  This increases the effectiveness of the developer by keeping her in the same branch of code.  The obvious downside is that you may have some duplication across branches, so make sure that there are ways to communicate across teams within the same discipline so that they can look for opportunities for reuse if it makes sense.  But, don't favor reuse for the sake of reuse knowing that it drives up complexity.

Second, consider whether you can have a single codebase.  This may be true in the case of moving to cloud services.  Or, you may have properties which configure the optional feature set.  The intent here would be to create a long-lived team which can handle a set of features.

Third, make sure that you are clear on priorities and drive down work in progress.  Focus on getting a few things done well in shippable form.  Any increase in work not completed creates complexity all the way across the value stream.

Dealing with Delays

Delays can have many ill effects - broken promises, problems with finding defects late in the cycle, added stress to the test team when they are blamed for everything being late.  But, it also creates cognitive stress as the team tries to balance competing work priorities.  Delays in the system enforces work reassignments immediately increasing work in progress which adds to the cognitive load of each member of the team that is affected by the delay.

This becomes a vicious cycle that leads to further delays in the system.  Delays affectively lead to complexity.

What is the remedy?

Aggressively attack those parts of your system that cause or increase delay.  This could be any number of problems, such as functional silos, long planning cycles, lack of customer interaction or not enough focus on early testing.

If you look at the problem from your customer's perspective, each of these delays causes them to wait.  For a time this could simply break the trust you have built with the customer, but in the long run they will turn angry or indifferent moving on to other suppliers.  A significant delay means that your solution will be behind the market and thus less meaningful for your customer.

Focusing on simplicity means having the mindset that keeping processes simple increases flow and throughput.  

Test First is Simpler

One of the practices we might want to adopt to simplify process would be test-first to reduce the need for longer debug cycles trying to sift through the code to find where the bugs where introduced.  Testing first increases the number of tests at the bottom of the test pyramid thus reducing the need for more complex combinatorial tests later in the cycle.  

Test first also creates a very tight feedback cycle.  It reduces the "hope and pray" tactics of manual testing efforts.  Most importantly, the reduced time means that context is not lost.  If a problem arises, it was only seconds or at most minutes after the bug was introduced.

Cross-functional Teams are Simpler

Cross-functional teams allow focused effort on a few items in the system at once.  There is shared context of the problem.  Even if you have a situation where there is not perfect knowledge across the team (and this is almost never achievable anyway), the shared context provides a tight feedback system that reduces overall complexity.  

Reduce work-in-progress however you are managing your process.  Keeping the whole team or a significant portion of the team on a single user story enforces collaboration and collectivism.  

Emergent Design is Simpler

Teams that come from a culture of waterfall built systems believe in identifying designs up front in order to reduce overall risk.  While this is still a good idea in general to think about system design up front, most knowledge is actually acquired as work is completed and shared so that many minds can reflect on the outcome.  Therefore, working in smaller batches identifying the need for design patterns and letting this emerge as a practice is the most beneficial to the system as it is shared knowledge and is proven through application.

Doing this the opposite way actually increases risk overall to the system since it is based on little understanding of the affect the feature set in the backlog will have on the system.  And, it drives up complexity as the understanding of the system is kept apart from the implementors. 

In the End

Focus on simplicity first if you want to improve overall outcomes.  There are many patterns and practices you can put into place other than what I have mentioned above.  Try something.  Inspect.  Adapt.  Communicate.  But, above all, keep it simple.

Code Refactoring - Dealing with Legacy Code (Part 2)

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.

1. Validation of the art type by URL suffix.
2. Image art retrieval.
3. Local storage of the art.
4. 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.

 
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.

Code Refactoring - Dealing with Legacy Code

If you find yourself dealing with legacy code, that probably means you have code that:

• Doesn't have a lot of tests (maybe none)
• Is difficult to modify
• Is not easily understandable
• Has tricky bits (takes a long time to debug certain areas)

I'm going to use a specific example from an open source codebase to show how to surgically inspect and adapt your code to make it yield to your bidding.

Now, for this example the code is in Java.  I'll follow up at some point with an example in native code.  One major advantage of working in Java is that there are no real compiler variations, which make it very easy to introduce tests.

Beware the long method

This example is a method which fetches album art from a URL and stores it in a local datastore.  The unaltered code is below.

private void saveCoverArt(String path, String url) throws Exception { 
        InputStream input = null; 

        OutputStream output = null;
        HttpClient client = new DefaultHttpClient();

        try {
            HttpConnectionParams.setConnectionTimeout(client.getParams(), 20 * 1000); // 20 seconds
            HttpConnectionParams.setSoTimeout(client.getParams(), 20 * 1000); // 20 seconds
            HttpGet method = new HttpGet(url);

            HttpResponse response = client.execute(method);
            input = response.getEntity().getContent();

            // Attempt to resolve proper suffix.
            String suffix = "jpg";
            if (url.toLowerCase().endsWith(".gif")) {
                suffix = "gif";
            } else if (url.toLowerCase().endsWith(".png")) {
                suffix = "png";
            }

            // Check permissions.
            File newCoverFile = new File(path, "cover." + suffix);
            if (!securityService.isWriteAllowed(newCoverFile)) {
                throw new Exception("Permission denied: " + StringUtil.toHtml(newCoverFile.getPath()));
            }

            // If file exists, create a backup.
            backup(newCoverFile, new File(path, "cover.backup." + suffix));

            // Write file.
            output = new FileOutputStream(newCoverFile);
            IOUtils.copy(input, output);

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

        } finally {
            IOUtils.closeQuietly(input);
            IOUtils.closeQuietly(output);
            client.getConnectionManager().shutdown();
        }
    }

Now, the issue with this code is not that it is functionally wrong. It may work just fine. But, it takes a few reads and eye movement up and down and possibly to other classes to understand what's going on.  The method is too long, and it is violating the Single Responsiblity Principle.

Start by Understanding the Current Behavior

To refactor this, I want to know what it's current behavior is.  The easiest way is by building a test.  I may want to build a few tests to understand all the paths through the code.  I will need to make one change (easily reversible) to remove the private accessor.

A couple of things that I notice that are going to be important are that 1) this is looking for an http based URL, and 2) there are three different file extensions supported for the cover art (.jpg, .gif, and .png).

So, let's start with the first test - a happy case.  

First Test - Download a PNG of Dark Side of the Moon

I don't want to modify the code under test at this point, I merely want to characterize it's behavior.  So, I'll do the naive thing and then adjust as necessary.

The above png is available on Wikipedia, so I can make sure my code is pointing to that url when I download it.  I want to test the saveCoverArt( String path, String url ) method.

Here's my first try at a test:

public void testSaveCoverArtForDarkSideofTheMoonUsingPng() throws Exception 
{
    String savePath = ".";
    String url = "http://upload.wikimedia.org/wikipedia/en/thumb/3/3b/Dark_Side_of_the_Moon.png/220px-Dark_Side_of_the_Moon.png";
    coverArtService.saveCoverArt(savePath, url);
    File f = new File(savePath,"cover.png");
    assertTrue(f.exists());
}

Now, because I have done the naive thing, I get a NullPointerException thrown when the test runs at line 94 of CoverArtService.

CoverArtService is expecting to have something called SecurityService defined.  Upon inspection of the class I see a setter for SecurityService, meaning they have implemented dependency injection.  This is a really important concept for testability.  

Dependency injection allows the implementation of class dependencies to be replaceable.  This means I can test it easily without changing the code.  If you look at the alternative point of view of code without dependency injection, then CoverArtService would have instantiated its own SecurityService either through the new keyword or through a Factory.  

Because I can replace this dependency, I will use a mocking framework to do so.  There are quite a few mocking frameworks for Java.  I'll stick with Mockito, because it is terse and doesn't come with some of the unnecessary baggage of other mocking frameworks.

Here's the test again, now with a service mock.

public void testSaveCoverArtForDarkSideofTheMoonUsingPng() throws Exception 
{
    String savePath = ".";
    String url = "http://upload.wikimedia.org/wikipedia/en/thumb/3/3b/Dark_Side_of_the_Moon.png/220px-Dark_Side_of_the_Moon.png";
    when(securityServiceMock.isWriteAllowed(notNull(File.class))).thenReturn(true);
    coverArtService.saveCoverArt(savePath, url);
    File f = new File(savePath,"cover.png");
    assertTrue(f.exists());
    verify(securityServiceMock).isWriteAllowed(notNull(File.class));
}

First, a bit of explanation about mocks.  There's some weird looking syntax here at the where ... thenReturn.  This allows me to affect the mock object's behavior and make it behave like a stub when the isWriteAllowed method is called.  Later, after the call to coverArtService.saveCoverArt, I want to verify that isWriteAllowed was called because I expected it to be.  If this is not the case, then the test will fail.

Bah!  I got the NullPointerException again!  Except this time, it's moved to line 105 where the MediaFileService is called.  So, I'll use mock again.  At this point, I'll skip ahead and show that I also created a mock for the MediaFile class.

public void testSaveCoverArtForDarkSideofTheMoonUsingPng() throws Exception 
{
    String savePath = ".";
    String url = "http://upload.wikimedia.org/wikipedia/en/thumb/3/3b/Dark_Side_of_the_Moon.png/220px-Dark_Side_of_the_Moon.png";
    MediaFile mediaFileMock = mock(MediaFile.class);
    when(securityServiceMock.isWriteAllowed(notNull(File.class))).thenReturn(true);
    when(mediaFileServiceMock.getMediaFile(savePath)).thenReturn(mediaFileMock);
    coverArtService.saveCoverArt(savePath, url);
    File f = new File(savePath,"cover.png");
    assertTrue(f.exists());
    verify(securityServiceMock).isWriteAllowed(notNull(File.class));
    verify(mediaFileServiceMock).getMediaFile(savePath);
}

Now, the test runs and I am happy.  Except that, it took too long to run.

Running net.sourceforge.subsonic.ajax.CoverArtServiceTest
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 3.971 sec

Of course, this is not a true unit test.  This is more like a functional test.  The reason is that I am testing things that don't have anything to do with the logic of the code.  For example, fetching graphics from somewhere online and persisting to file don't actually need to be run in a unit test.  In fact, they shouldn't be.  Since this test took nearly 4 seconds to run, imagine what would happen if I had a hundred of these types of tests.  Goodbye fast feedback.  So, I want to reduce my dependency on I/O.  

And, another thing.  I have only scraped the surface of the behavior of this method.  So, I will add some other types of tests before I start refining the code too much.

Refactor

I have green tests (working tests).  Now, it's time to refactor the code.  If I take a look at the top section of the code, I see that it's really about setting up a connection.  So let's create a new method for this functionality.

Tip #1: Start small.

Besides starting small, I also want to use copy/paste rather than cut/paste.  If I cut/paste, I may end up in a situation where my code doesn't pass tests anymore and that's not the point.  The point is to use the safety net to make sure I'm not making mistakes by getting too far ahead of myself.

So, I will copy out the code needed to setup the connection and extract a method for it.

HttpClient getHttpClient()
{
    HttpClient client = new DefaultHttpClient();
    HttpConnectionParams.setConnectionTimeout(client.getParams(), 20 * 1000);
    HttpConnectionParams.setSoTimeout(client.getParams(), 20 * 1000);
    return client;
}

Now, before removing the equivalent from the original method, compile this.  Okay, now we can move forward.

void saveCoverArt(String path, String url) throws Exception {
        InputStream input = null;
        OutputStream output = null;
        HttpClient client = getHttpClient();

        try {
            HttpGet method = new HttpGet(url);
...

Our code looks slightly cleaner, but we should probably have a strategy moving forward.  Let's take another look at the method overall.  I won't repeat it here, but feel free to scroll back up.

There are a few discrete operations that are happening in this method.  

1. Image art is being downloaded from an online HTTP based resource.
2. Validation of the art type (jpg, png, gif).
3. The art is being persisted to file store after passing security.
4. The MediaFileService is getting updated if this is a new version of the original cover art.

So, let's use these as a way to refactor this method.

First, it looks like these steps are out of order.  Why is the suffix being validated after we have fetched the online resource?  What happens if the suffix is invalid?  It looks like there is an incorrect assumption in the code that anything that is not a gif or png is a jpg.  So, there are a few things to fix here.  Let's start by extracting the validation step.

Tip #2: Reorder the steps if necessary

I'll create a method for the image type validation.

First, I'm going to create a simple enum to hold the valid types of images.

public enum ImageType
{
    JPG,
    GIF,
    PNG
}

Second, I'll create the method that will return the valid image type.

ImageType validateImageType(String stringToMatch)
{
    for (ImageType imageType : ImageType.values() )
    {
        if (stringToMatch.toUpperCase().endsWith(imageType.name()))
            return imageType;
    }
    return null;
}

Now, we can create a test for this behavior.  

A special note.

Ordinarily, we would create the test first and then drive the behavior.  However, when refactoring code without unit tests, this is often difficult to do.  Instead, we can extract testability from the existing code and create tests where there were none before.  In a later post, I'll show how to create the test first when refactoring.

Okay, here's the test for the success case.

@Test
public void testValidateImageTypeIsMatchedForPngUrl() throws Exception
{
    ImageType type = coverArtService.validateImageType(DARK_SIDE_PNG_URL);
    assertEquals(ImageType.PNG, type);
}

We should also have a test for an invalid value.

@Test
public void testInvalidImageTypeIsMatchedForBmpUrl() throws Exception
{
    ImageType type = coverArtService.validateImageType(DARK_SIDE_BMP_URL);
    assertNull(type);
}

Now, let's put it together.

Here's the updated 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;
    }
    String suffix = imageType.name().toLowerCase();
        
    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();

        /* persistCover( path, suffix, input ) */
        File newCoverFile = new File(path, "cover." + suffix);
        if (!securityService.isWriteAllowed(newCoverFile)) {
            throw new Exception("Permission denied: " + StringUtil.toHtml(newCoverFile.getPath()));
        }

        backup(newCoverFile, new File(path, "cover.backup." + suffix));

        // Write file.
        output = new FileOutputStream(newCoverFile);
        IOUtils.copy(input, output);

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

    } finally {
        IOUtils.closeQuietly(input);
        IOUtils.closeQuietly(output);
        client.getConnectionManager().shutdown();
    }
}

Unit Tests are Faster

Now, because I have refactored the tests you can see a huge difference between the amount of time it took to run the original functional test and the unit tests that are doing string validation.

By doing more refactoring, I can get a much faster test suite focusing on using mocks and removing the external dependencies that are I/O bound.  We are going to trust that I/O works.  We're using the Java standard library after all.

This is (not) the end

I have gone through a simple way to introduce tests and begin to refactor a lengthy method.  However, I'm not done.  In the next post, I'll carry on with this refactor and discover mistakes I have made and correct those too.

...to be continued.

Breaking Down Stories using ATDD

On a recent trip, I was asked how one could go about breaking down a feature request into something that was small enough to consume by the team, but that also had meaning at completion?

Let me frame this question a bit.  This was asked by someone in an organization made up of component teams that is being asked to organize for features instead.

If you are lucky and have never been in a component-based organization, then here's a primer.  In a component-based organization there exists the thinking such that if I put all of the people with specialized skills together then they can share knowledge with each other and that part of the system will have great quality and integrity because everyone working in that team will have deep knowledge which is extremely hard to obtain in a large organization.

Unfortunately, what happens is that people end up losing the big picture.  Thus, knowledge is lost, not gained.  What each part of the system provides is probably optimal for that part of the system, but may not integrate very well overall with other parts of the system.  At least, they may not know until too late when integration time comes around.  Then, an expedient choice is made due to time pressures, code becomes tightly coupled for the sake of getting the product out the door and a promise is made to refactor the code later. 

Of course, we would like to avoid sad baby.  So, if we really want to figure out how to break down stories in a meaningful way, how can we do it?  

Focus on the Goal

What is the goal of a story?  Ultimately, it's to deliver something that can be used to generate feedback.  This doesn't mean the same thing every time.  I like to think of this as a continuum.

Looking at the left side of this continuum you can think of the outcome of the story as mainly feedback for the team.  The feature is not ready to publish yet.  For instance, we might want to vet a feature workflow by focusing mainly on UI components without a real backend.  This is the lean thinking prevalent in the Lean Startup.  

Whether the goal of the story is just research, or whether it's targeted to a simple scenario such as single user or it's on the other end of the spectrum and intended to be deployed to the customer, we can use the outcome to give us some data that can qualitatively prove we have done the right thing or the wrong thing.  This is one of the main reasons for making stories small in the first place.  You want to fail fast and early!

The Benefits of the Small

There are many benefits to working in small batches.  You probably already know that it normalizes your workload, makes it easier to estimate your work and makes it easier to finish.  But, there's another very important reason to make things smaller that is often overlooked that I want to explore.

Working in the small will deliberately make your code more modular and testable.  Just think about it.  How many times have you been in the situation where you were developing some code and someone depending on your code wanted to see it.  But, you may have been reluctant because you didn't quite have all the features finished and you just wanted to handle this one last use case.  Then, when you did finally hand it over it turned out that your assumptions about some things were incorrect and you overproduced.  Now, you have to go back and make some changes, but you ignore the parts that were overproduced.  Some time later, someone else on the team has to work with it and can't sort out why there are extra methods and classes, but doesn't have time to fix it so they just add what's needed.  Before long, the complexity is up and you have become the subject matter expert.

Instead, if you focus on getting a piece of code done in a few hours, no more than half a day with tests this will demand that you simplify.  In fact, you will need some tools in your toolbox to help you.  The first tool I want to explore is acceptance test driven development (ATDD).

Breaking Down Stories the ATDD Way

When creating tasks for a story, it often helps to think about the goal and where on the continuum my goal lies.  To help discover this, think of the acceptance tests first.

Let's take a simple example.  For a VOIP phone application, I may have a feature called "Make a call" that is considered a must have feature.  Now, there are many aspects to make a call that I need to consider:

• Is this a local or long distance call?
• Should I handle international calling?
• Am I dialing via inter-office dial rules?
• Is the remote party a regular PSTN phone or is it another VOIP phone?
• Do I need to display calling name & number?
• What voice codecs do I need to support?

These are just some examples of questions that may arise.  What you should then do is simplify the story to a subset that is demonstrable within the iteration time frame.  Perhaps this would be 

Make a local call using 7 digits to a regular PSTN phone with the G.711 codec.

Now that you have constrained the requirement, let's further take into account some acceptance tests.

1. If the digits are not a valid PSTN number, then I should hear a reorder signal.
2. If the called party is active on all lines, then I should hear a busy signal.
3. If the called party does not answer, then I should continue to hear ringing until I hang up (assume no voicemail).
4. If the called party answers, then I should hear 2-way audio.
5. Hanging up ends the 2-way audio stream.

Now you have some scenarios that can help you create tasks for this story.  Obviously, there are tests to write, so there are at least 5 tasks there.  Let's just look at one and see what other tasks might evolve.

• Task: Create a test to validate reorder signal when dialed party is an invalid number.
• Task: Find a way to match reorder signal through an automated system.
• Task: Verify SIP return codes are handled.
• Task: Create code to handle protocol return codes 404 and 480 and present useful information to user.
• Task: UX flows.

Note that the above tasks don't say things that you normally do like code review, check-in code, do designs.  These are things you have to do anyway.  There's no need to point them out.

ATDD - The Group Way

It's a great idea to make this the way you do Sprint Planning.  Come up with your tests and let that drive your planning session.  This way, test is involved up front as a first class citizen and everyone is on the same page as to what done means for your user story.