A Real World Example of Refactoring

I’m leading an agile team through developing a web site. This means that I spend most of my time managing, but on this one occasion I had the opportunity to write some code.

The problem

We had an image stored in a database that was always either 800×600 (portrait) or 600×800 (landscape). We had a need to render that image on the site either as-is or reduced to one of two other sizes. You can think of this as a thumbnail, a details image, and a full-size image. We were (of course) writing the code test first, and the tests were focusing on getting the sizes right and not so much on checking the generated images. We ended up getting it working, but we were thoroughly disgusted with the code that we produced 🙂 At least we knew the code was bad, and resolved to fix it when we had a chance.

Before we get into the application code, let’s look at the tests we wrote, and I’ll explain the evolution of the code and what the classes involved are.

As you can see, the class under test is called ImageWriter. It came into being because we were being careful about resource management, so we didn’t want to resize an image and expose it to the world, where it might not get disposed. So, our concept was to create this class, whose purpose in life is to write a properly sized image to a Stream. It would ensure that the image was sized correctly, it was put into the stream properly, and the resources were reclaimed. Sounds pretty simple, and it was, other than some ugly switch logic.

We wrote the first three tests you see below first, starting with just taking in the full-sized landscape image and writing that to the stream. This wasn’t a hard test to get working, as you might expect. We followed that up with writing the detail-sized image, which forced us to write a conditional statement to choose between the two sizes. And then we wrote the third test, which caused us to write another else to allow us to choose thumbnail-sized images. At the fourth test, it started to get really ugly when we had to decide if the image was portrait or landscape, which added a totally different conditional statement. Very rapidly this code was becoming unwieldy. We quickly wrote the fifth and sixth tests, just to get the functionality working, since we were just following an already existing pattern, ugly though it was. Once we were finished, though, we knew we needed to refactor this beast before checking it in.

7 [TestFixture]

8 public class ImageWriterFixture

9 {

10 private Image fullSizeLandscapeImage;

11 private Image fullSizePortraitImage;

12 private MemoryStream imageInStream;


14 [SetUp]

15 public void SetUp()

16 {

17 fullSizeLandscapeImage = new Bitmap(800, 600);

18 fullSizePortraitImage = new Bitmap(600, 800);

19 imageInStream = new MemoryStream();

20 }


22 [TearDown]

23 public void ReleaseResources()

24 {

25 imageInStream.Dispose();

26 fullSizeLandscapeImage.Dispose();

27 }


29 [Test]

30 public void ImageWriteWillWriteFullSizeLandscapeImages()

31 {

32 ImageWriter writer = ImageWriter.GetFullSizeWriter(imageInStream);

33 writer.Write(fullSizeLandscapeImage);


35 Image rereadImage = ResizeImageFromStream();


37 Assert.AreEqual(fullSizeLandscapeImage.Height, rereadImage.Height);

38 Assert.AreEqual(fullSizeLandscapeImage.Width, rereadImage.Width);

39 }


41 private Image ResizeImageFromStream()

42 {

43 imageInStream.Seek(0, SeekOrigin.Begin);

44 return Image.FromStream(imageInStream);

45 }


47 [Test]

48 public void ImageWriterWillWriteDetailsLandscapeImages()

49 {

50 ImageWriter writer = ImageWriter.GetDetailsSizeWriter(imageInStream);

51 writer.Write(fullSizeLandscapeImage);


53 Image rereadImage = ResizeImageFromStream();


55 Assert.AreEqual(306, rereadImage.Height);

56 Assert.AreEqual(408, rereadImage.Width);

57 }


59 [Test]

60 public void ImageWriterWillWriteThumbnailLandscapeImages()

61 {

62 ImageWriter writer = ImageWriter.GetThumbnailSizeWriter(imageInStream);

63 writer.Write(fullSizeLandscapeImage);


65 Image rereadImage = ResizeImageFromStream();


67 Assert.AreEqual(105, rereadImage.Height);

68 Assert.AreEqual(140, rereadImage.Width);

69 }


71 [Test]

72 public void ImageWriterWillWriteFullSizePortraitImages()

73 {

74 ImageWriter writer = ImageWriter.GetFullSizeWriter(imageInStream);

75 writer.Write(fullSizePortraitImage);


77 Image rereadImage = ResizeImageFromStream();


79 Assert.AreEqual(800, rereadImage.Height);

80 Assert.AreEqual(600, rereadImage.Width);

81 }


83 [Test]

84 public void ImageWriterWillWriteDetailsSizePortraitImages()

85 {

86 ImageWriter writer = ImageWriter.GetDetailsSizeWriter(imageInStream);

87 writer.Write(fullSizePortraitImage);


89 Image rereadImage = ResizeImageFromStream();


91 Assert.AreEqual(306, rereadImage.Height);

92 Assert.AreEqual(230, rereadImage.Width);

93 }


95 [Test]

96 public void ImageWriterWillWriteThumbnailSizePortraitImages()

97 {

98 ImageWriter writer = ImageWriter.GetThumbnailSizeWriter(imageInStream);

99 writer.Write(fullSizePortraitImage);


101 Image rereadImage = ResizeImageFromStream();


103 Assert.AreEqual(105, rereadImage.Height);

104 Assert.AreEqual(79, rereadImage.Width);

105 }


107 }

Now here is the finished, but mostly unrefactored, source code. During the process of writing the tests, we did do some refactoring to clean up the code a bit, make things a bit more readable, etc, but we held off on the Replace Conditional with Polymorphism refactoring that we could both see coming. And that refactoring is what I want to eventually share here. So, here is our ugly code:

1 public class ImageWriter : IImageWriter

2 {

3 private Stream stream;

4 private readonly ImageSize desiredImageSize;

5 private int desiredHeight;

6 private int desiredWidth;


8 private enum ImageSize

9 {




13 } ;


15 private readonly Rectangle LandscapeFullSize = new Rectangle(0, 0, 800, 600);

16 private readonly Rectangle LandscapeDetailsSize = new Rectangle(0, 0, 408, 306);

17 private readonly Rectangle LandscapeThumbnailSize = new Rectangle(0, 0, 140, 105);

18 private readonly Rectangle PortraitFullSize = new Rectangle(0, 0, 600, 800);

19 private readonly Rectangle PortraitDetailsSize = new Rectangle(0,0, 230, 306);

20 private readonly Rectangle PortraitThumbnailSize = new Rectangle(0, 0, 79, 105);


22 public static ImageWriter GetFullSizeWriter(Stream imageInStream)

23 {

24 return new ImageWriter(imageInStream, ImageSize.FULLSIZE);

25 }


27 public static ImageWriter GetDetailsSizeWriter(Stream imageInStream)

28 {

29 return new ImageWriter(imageInStream, ImageSize.DETAILS);

30 }


32 public static ImageWriter GetThumbnailSizeWriter(Stream imageInStream)

33 {

34 return new ImageWriter(imageInStream, ImageSize.THUMBNAIL);

35 }


37 private ImageWriter(Stream stream, ImageSize imageSize)

38 {

39 this.stream = stream;

40 desiredImageSize = imageSize;

41 }


43 public void Write(Image image)

44 {

45 int width = image.Width;

46 int height = image.Height;


48 if (width < height) // isPortrait

49 {

50 switch(desiredImageSize)

51 {

52 case ImageSize.FULLSIZE:

53 desiredHeight = PortraitFullSize.Height;

54 desiredWidth = PortraitFullSize.Width;

55 break;


57 case ImageSize.DETAILS:

58 desiredHeight = PortraitDetailsSize.Height;

59 desiredWidth = PortraitDetailsSize.Width;

60 break;


62 case ImageSize.THUMBNAIL:

63 desiredHeight = PortraitThumbnailSize.Height;

64 desiredWidth = PortraitThumbnailSize.Width;

65 break;

66 }

67 }

68 else // isLandscape

69 {

70 switch(desiredImageSize)

71 {

72 case ImageSize.FULLSIZE:

73 desiredHeight = LandscapeFullSize.Height;

74 desiredWidth = LandscapeFullSize.Width;

75 break;


77 case ImageSize.DETAILS:

78 desiredHeight = LandscapeDetailsSize.Height;

79 desiredWidth = LandscapeDetailsSize.Width;

80 break;


82 case ImageSize.THUMBNAIL:

83 desiredHeight = LandscapeThumbnailSize.Height;

84 desiredWidth = LandscapeThumbnailSize.Width;

85 break;

86 }

87 }


89 using (Image resized = image.GetThumbnailImage(desiredWidth, desiredHeight, null, IntPtr.Zero))

90 {

91 resized.Save(stream, ImageFormat.Jpeg);

92 }

93 }

94 }

The bright idea we had while we were writing it was that we would use the Rectangles to hold the dimensions of an image of the proper size, to make it easier to identify what the magic numbers for height and width meant. This also made it easier for us to write the body of each leg of the case statements. Clearly, however, this was only a short term workaround for a more proper solution later.

Beginning the refactoring

OK, so we’re about to start this. I truly have never attempted the refactoring that we’re going to try here on this code, so I’m going to be doing it essentially live for you. I’ll try to share with you any mistakes I make, what thoughts are going through my semi-sentient head, and what I’m feeling about the code as it progresses. Our goal is to end up in a situation where any conditional behavior is moved out of a procedural if/then/else block and into some sort of polymorphic dispatch, but to do that in small, orderly steps, such that we’re always pretty close to having working code.

To begin, I’m planning on opening up my refactoring book to the section on Replace Conditional with Polymorphism. As I tell my students in every TDD course I teach, please open your Fowler Refactoring books and follow the steps, as Martin makes these things easy once you figure out which refactoring to use. So, to follow my own advice, I’m going to open the book and use it as I go.

First step — Are the responsibilities in the right place?

The first thing I notice when I look at the ImageWriter’s Write method is that I see policy and details happening in the same place. The policy in that class can be summed up as, “Determine the dimensions of the final image, resize the image, and then write the image to the stream”, and the details in that method are concerned with how those dimensions are determined. In order for us to do anything at all to simplify this system, we’re going to have to pull the dimension calculations out into another method at least, and into another class possibly after that. So lets start with an ExtractMethod refactoring to get those dimension calculations out of there.

As my first step in doing this, I noticed that the member variables desiredHeight and desiredWidth weren’t really doing anything good for me, and I could get rid of them by using height and width, the local variables declared in the Write method, in their place, as such:

1 public void Write(Image image)

2 {

3 int width = image.Width;

4 int height = image.Height;


6 if (width < height) // isPortrait

7 {

8 switch(desiredImageSize)

9 {

10 case ImageSize.FULLSIZE:

11 height = PortraitFullSize.Height;

12 width = PortraitFullSize.Width;

13 break;


15 case ImageSize.DETAILS:

16 height = PortraitDetailsSize.Height;

17 width = PortraitDetailsSize.Width;

18 break;


20 case ImageSize.THUMBNAIL:

21 height = PortraitThumbnailSize.Height;

22 width = PortraitThumbnailSize.Width;

23 break;

24 }

25 }

and so on. I think this sets me up very nicely to get rid of the individual width and height variables and replacing them with a Rectangle object. I’m going to try that in the code and see where that takes me. I’m not going to make this whole change all at once, because that’s too large of a change. Instead, I’m going to find a way to refactor each leg of the switch to contain the assignment to the desiredRectangle reference and then take advantage of that rectangle to set the height and width repeatedly.

48 public void Write(Image image)

49 {

50 Rectangle desiredRectangle;


52 int width = image.Width;

53 int height = image.Height;


55 if (width < height) // isPortrait

56 {

57 switch(desiredImageSize)

58 {

59 case ImageSize.FULLSIZE:

60 desiredRectangle = PortraitFullSize;

61 height = desiredRectangle.Height;

62 width = desiredRectangle.Width;

63 break;

As you can see, I added a new Rectangle reference on line 50 which is going to hold the rectangle with the desired dimensions. And the smallest change I could make to start making use of this was to rewrite the body of the case statement starting on line 60 as you can see. For those of you new to refactoring, this is one of the most important pieces of the process — take steps that are as small as possible. By doing this, you keep your risk down as low as possible while you’re changing your code. If you take big steps and mess something up, it could take you a lot of time to get back to having something working. If you take a small step and mess something up, you can just back up a bit to where things worked. I’m taking a small step here, and just changing this leg of the switch. And after doing this, I ran my tests, and they worked. I’m going to change the rest of the legs now, running my tests between each change. I’ll do this privately, as it doesn’t seem very interesting to show you each step along this way.

OK, I did that, and all my tests worked, and each of the legs of the switches looks just like the sample code above, except for a different equivalent of line 60 for each case. Now that I’ve done this, I believe I can factor out the setting of the height and width in each leg and do that at the bottom of the method, right before actually doing the resizing. That will leave the code looking like this:

48 public void Write(Image image)

49 {

50 Rectangle desiredRectangle = new Rectangle();


52 if (image.Width < image.Height) // isPortrait

53 {

54 switch(desiredImageSize)

55 {

56 case ImageSize.FULLSIZE:

57 desiredRectangle = PortraitFullSize;

58 break;


60 case ImageSize.DETAILS:

61 desiredRectangle = PortraitDetailsSize;

62 break;


64 case ImageSize.THUMBNAIL:

65 desiredRectangle = PortraitThumbnailSize;

66 break;

67 }

68 }

69 else // isLandscape

70 {

71 // extra stuff elided

85 }


87 int height = desiredRectangle.Height;

88 int width = desiredRectangle.Width;


90 using (Image resized = image.GetThumbnailImage(width, height, null, IntPtr.Zero))

91 {

92 resized.Save(stream, ImageFormat.Jpeg);

93 }

94 }

95 }

So each leg of the switches has become more simple and we’re breaking out the height and width individually now only at the end. During the next refactoring, I’ll probably do an Inline refactoring on height and width, as they’re really not helping much, which would shrink this method down even more.

Now that we’re at this point, I think I can do the ExtractMethod I talked about previously on the switch stuff and move that out into another method, so we can make the Write method only concerned with the higher level, more abstract steps of how this process works, and get the details of how the dimensions are calculated into its own method. After this refactoring, another ExtractMethod to take the mechanics of the writing to the stream out, and a couple of renamings to clarify what the rectangle dimension calculations actually mean, Write looks like this, which is just about right 🙂

48 public void Write(Image image)

49 {

50 Rectangle imageDimensions = CalculateScaledImageDimensions(image);

51 WriteScaledImage(image, imageDimensions);

52 }

A decision needs to be made

I’m at somewhat of a crossroads here. In looking back at the Write method, I’ve decided I really don’t like it. Something just seems strange to me about it. I know I need to do something with scaled images, but instead I’m working with the dimensions of that scaled image. To me, the calculations of the dimensions of the scaled images and storing those dimensions into a rectangle seems like an implementation detail of how I did this. What the code really needs to be using, and written in terms of, are the scaled images themselves. In doing this, I think the method will now read better, since both lines of code in it will be using the same abstraction. Instead of using a Rectangle to represent something about the scaled image, now I can deal with the scaled image throughout the method. I like this better. This leads us to this code:

48 public void Write(Image image)

49 {

50 using (Image scaledImage = GenerateScaledImage(image))

51 {

52 WriteScaledImage(scaledImage);

53 }

54 }

I like this a lot better, as it seems like both of the lines of this method are dealing with the same abstraction now, a ScaledImage. This leads me to think that there is a ScaledImage class or hierarchy of classes trying to find its way out, which was our goal when we started this — we were looking for the right place to put our polymorphic logic, and this ScaledImage class seems like the right place. The final code for this class looks like this:

8 public class ImageWriter : IImageWriter

9 {

10 private Stream stream;

11 private readonly ImageSize desiredImageSize;


13 private enum ImageSize

14 {




18 } ;


20 private readonly Rectangle LandscapeFullSize = new Rectangle(0, 0, 800, 600);

21 private readonly Rectangle LandscapeDetailsSize = new Rectangle(0, 0, 408, 306);

22 private readonly Rectangle LandscapeThumbnailSize = new Rectangle(0, 0, 140, 105);

23 private readonly Rectangle PortraitFullSize = new Rectangle(0, 0, 600, 800);

24 private readonly Rectangle PortraitDetailsSize = new Rectangle(0,0, 230, 306);

25 private readonly Rectangle PortraitThumbnailSize = new Rectangle(0, 0, 79, 105);


27 public static ImageWriter GetFullSizeWriter(Stream imageInStream)

28 {

29 return new ImageWriter(imageInStream, ImageSize.FULLSIZE);

30 }


32 public static ImageWriter GetDetailsSizeWriter(Stream imageInStream)

33 {

34 return new ImageWriter(imageInStream, ImageSize.DETAILS);

35 }


37 public static ImageWriter GetThumbnailSizeWriter(Stream imageInStream)

38 {

39 return new ImageWriter(imageInStream, ImageSize.THUMBNAIL);

40 }


42 private ImageWriter(Stream stream, ImageSize imageSize)

43 {

44 this.stream = stream;

45 desiredImageSize = imageSize;

46 }


48 public void Write(Image image)

49 {

50 using (Image scaledImage = GenerateScaledImage(image))

51 {

52 WriteScaledImage(scaledImage);

53 }

54 }


56 private void WriteScaledImage(Image scaledImage)

57 {

58 scaledImage.Save(stream, ImageFormat.Jpeg);

59 }


61 private Image GenerateScaledImage(Image image)

62 {

63 Rectangle imageDimensions = new Rectangle();


65 if (image.Width < image.Height) // isPortrait

66 {

67 switch(desiredImageSize)

68 {

69 case ImageSize.FULLSIZE:

70 imageDimensions = PortraitFullSize;

71 break;


73 case ImageSize.DETAILS:

74 imageDimensions = PortraitDetailsSize;

75 break;


77 case ImageSize.THUMBNAIL:

78 imageDimensions = PortraitThumbnailSize;

79 break;

80 }

81 }

82 else // isLandscape

83 {

84 switch(desiredImageSize)

85 {

86 case ImageSize.FULLSIZE:

87 imageDimensions = LandscapeFullSize;

88 break;


90 case ImageSize.DETAILS:

91 imageDimensions = LandscapeDetailsSize;

92 break;


94 case ImageSize.THUMBNAIL:

95 imageDimensions = LandscapeThumbnailSize;

96 break;

97 }

98 }


100 return image.GetThumbnailImage(imageDimensions.Width, imageDimensions.Height, null, IntPtr.Zero);

101 }

102 }

Next time

This entry is getting pretty long, so I’m going to end it here. When I pick it up again next time, I’ll do the refactoring to pull the conditional logic out into the ScaledImage hierarchy we’re going to create and see where the code takes us after that. I suspect that the WriteScaledImage method is going to find its way into there as well, given its name. One big hint that methods want to be grouped together is when you discover that they have a common abstraction as part of their name. GenerateScaledImage and WriteScaledImage seem to both be crying out to be in a ScaledImage class to me, but we’ll have to see.

I’m so sorry that I haven’t posted any worthwhile content in a long time, but I was tied up managing a huge waterfall-ish project all fall. That project is over, and I’m working with several different agile teams now with varying levels of involvement, which should give me more time to blog. I’m also working on 4 different proposals for Agile 2007 and one for the PMI National Congress. More on those as they get more fully formed.

As always, if you’ve made it this far, thanks for reading, and please let me know if you have any comments. I’ve had to disable comments on the blog as the spammers have taken over the comment logs, so send the emails to me directly. I’ll post a summary of the best questions and my answers in my next post.

— bab