Without Precedence

Writing Clean Code

by Morten Christiansen on 09-01-2010 at 18:19 | comments [1] | posted in Programming, Best Practices

Every once in a while, you come across a book that really resonates with you. For me, this recently happened with the book Clean Code: A Handbook of Agile Software Craftsmanship, by Robert C. Martin and Co. The book is all about recognizing, writing and modifying readable and maintainable code, from an agile perspective with a particular focus on tests and test-driven development. The message it brings is quite simple: It is not enough to create working code in order to succeed in the long run, you need to build high quality software or be crushed under the weight of code that is hard if not impossible to modify and maintain. The proposed solution is a series of craftsmanship principles and best practices, backed by plenty of examples.

Clean Code cover

Clean Code is one of the best programming books I've read in a long while. It is based on many of the same guiding principles I have for software development, such as the central role of readable code and the need to continually improve yourself as a craftsman, beyond the standard requirements of your job.

Some of the practices presented in the book weren't new to me and, indeed, some of them were a bit too idealistic, but apart from that it was truly an insightful read.

One of the more eye opening concepts was a way to write code so that it can be read much like you would read a newspaper. The first step in this process is to separate your code into lots of small functions. Essentially there should be no functions larger than about 4 lines and they should do one thing and one thing only. This thing should be captured accurately and unambiguously in the function name. A function should read like a sentence - to do x (the function name), you must do a, b and c (lines of code). Note that it is important that the contents of the function should be at the same level of abstraction and that this level of abstraction should be one level lower than that of the function name. The point to all of this is to make it trivially easy to see what a function does. Consider the following code I recently wrote for resizing images:

  1. public enum ResizeStrategy
  2. {
  3.     Stretch,
  4.     BestFit,
  5. }
  6.  
  7. public static class ImageFormatter
  8. {
  9.     public static Image ScaleImage(Image originalBitmap, int maxDimension)
  10.     {
  11.         float aspectX = maxDimension / (float)originalBitmap.Width;
  12.         float aspectY = maxDimension / (float)originalBitmap.Height;
  13.         float actualAspect = Math.Min(aspectX, aspectY);
  14.         int sourceWidth = (int)(originalBitmap.Width * actualAspect);
  15.         int sourceHeight = (int)(originalBitmap.Height * actualAspect);
  16.         var resized = new Bitmap(sourceWidth, sourceHeight);
  17.         Graphics g = Graphics.FromImage(resized);
  18.         g.DrawImage(originalBitmap, new Rectangle(0, 0, resized.Width, resized.Height));
  19.         return resized;
  20.     }
  21.  
  22.     public static Image ScaleImage(Image originalImage, float scale)
  23.     {
  24.         int width = (int)(originalImage.Width * scale);
  25.         int height = (int)(originalImage.Height * scale);
  26.         var resized = new Bitmap(width, height);
  27.         Graphics g = Graphics.FromImage(resized);
  28.         g.DrawImage(originalImage, new Rectangle(0, 0, resized.Width, resized.Height));
  29.         return resized;
  30.     }
  31.  
  32.     public static Image ScaleImage(Image originalImage, float scaleWidth, float scaleHeight)
  33.     {
  34.         int width = (int)(originalImage.Width * scaleWidth);
  35.         int height = (int)(originalImage.Height * scaleHeight);
  36.         var resized = new Bitmap(width, height);
  37.         Graphics g = Graphics.FromImage(resized);
  38.         g.DrawImage(originalImage, new Rectangle(0, 0, resized.Width, resized.Height));
  39.         return resized;
  40.     }
  41.  
  42.     public static Image ResizeImage(Image originalImage, int width, int height, ResizeStrategy thumbnailFormat)
  43.     {
  44.         Image thumbnail = new Bitmap(width, height);
  45.         switch (thumbnailFormat)
  46.         {
  47.             case ResizeStrategy.Stretch:
  48.                 RenderStretchedThumbnail(originalImage, thumbnail);
  49.                 break;
  50.             case ResizeStrategy.BestFit:
  51.                 RenderBestFitThumbnail(originalImage, thumbnail);
  52.                 break;
  53.             default:
  54.                 throw new ArgumentException("Thumbnail format not valid");
  55.         }
  56.         return thumbnail;
  57.     }
  58.  
  59.     private static void RenderBestFitThumbnail(Image originalImage, Image thumbnail)
  60.     {
  61.         float aspectX = thumbnail.Width / (float)originalImage.Width;
  62.         float aspectY = thumbnail.Height / (float)originalImage.Height;
  63.         float actualAspect = Math.Max(aspectX, aspectY);
  64.         int sourceWidth = (int)(thumbnail.Width / actualAspect);
  65.         int sourceHeight = (int)(thumbnail.Height / actualAspect);
  66.         Graphics g = Graphics.FromImage(thumbnail);
  67.         g.DrawImage(originalImage, new Rectangle(0, 0, thumbnail.Width, thumbnail.Height), new Rectangle(0, 0, sourceWidth, sourceHeight), GraphicsUnit.Pixel);
  68.     }
  69.  
  70.     private static void RenderStretchedThumbnail(Image originalImage, Image thumbnail)
  71.     {
  72.         Graphics g = Graphics.FromImage(thumbnail);
  73.         g.DrawImage(originalImage, 0, 0, thumbnail.Width, thumbnail.Height);
  74.     }
  75. }

I don't consider this code bad in any particular way, but lets see what happens if we rewrite it, using the above guidelines:

  1. public enum ResizeStrategy
  2. {
  3.     Stretch,
  4.     BestFit
  5. }
  6.  
  7. public class ImageFormatter
  8. {
  9.     private Image _workingImage;
  10.     private Image _originalImage;
  11.  
  12.     public Image ResizeImage(Image originalImage, int width, int height, ResizeStrategy resizeStrategy)
  13.     {
  14.         _originalImage = originalImage;
  15.  
  16.         PrepareImage(width, height);
  17.         switch (resizeStrategy)
  18.         {
  19.             case ResizeStrategy.Stretch:
  20.                 RenderStretchedImage();
  21.                 break;
  22.             case ResizeStrategy.BestFit:
  23.                 RenderBestFitThumbnail();
  24.                 break;
  25.             default:
  26.                 throw new ArgumentException("Thumbnail format not valid");
  27.         }
  28.         return _workingImage;
  29.     }
  30.  
  31.     private void PrepareImage(int width, int height)
  32.     {
  33.         _workingImage = new Bitmap(width, height);
  34.     }
  35.  
  36.     private void RenderStretchedImage()
  37.     {
  38.         Graphics g = Graphics.FromImage(_workingImage);
  39.         g.DrawImage(_originalImage, GetTargetDrawingRectangle());
  40.     }
  41.  
  42.     private Rectangle GetTargetDrawingRectangle()
  43.     {
  44.         return new Rectangle(0, 0, _workingImage.Width, _workingImage.Height);
  45.     }
  46.  
  47.     private void RenderBestFitThumbnail()
  48.     {
  49.         float scale = CalculateMaximumScaleNotCroppingBothDimensions();
  50.         Rectangle source = CalculateRectangleFromScaledWorkingImage(scale);
  51.         RenderPartialImage(source);
  52.     }
  53.  
  54.     private float CalculateMaximumScaleNotCroppingBothDimensions()
  55.     {
  56.         float xScale = _workingImage.Width / (float)_originalImage.Width;
  57.         float yScale = _workingImage.Height / (float)_originalImage.Height;
  58.         return Math.Max(xScale, yScale);
  59.     }
  60.  
  61.     private Rectangle CalculateRectangleFromScaledWorkingImage(float scale)
  62.     {
  63.         int width = (int)(_workingImage.Width / scale);
  64.         int height = (int)(_workingImage.Height / scale);
  65.         return new Rectangle(0, 0, width, height);
  66.     }
  67.  
  68.     private void RenderPartialImage(Rectangle source)
  69.     {
  70.         Graphics g = Graphics.FromImage(_workingImage);
  71.         g.DrawImage(_originalImage, GetTargetDrawingRectangle(), source, GraphicsUnit.Pixel);
  72.     }
  73.  
  74.     public Image ResizeImage(Image originalImage, int maxDimension)
  75.     {
  76.         _originalImage = originalImage;
  77.         float scale = CalculateScaleForFixedMaxDimension(maxDimension);
  78.         return ScaleImage(originalImage, scale);
  79.     }
  80.  
  81.     private float CalculateScaleForFixedMaxDimension(int lengthOfMaxDimension)
  82.     {
  83.         float xScale = lengthOfMaxDimension / (float)_originalImage.Width;
  84.         float yScale = lengthOfMaxDimension / (float)_originalImage.Height;
  85.         return Math.Min(xScale, yScale);
  86.     }
  87.  
  88.     public Image ScaleImage(Image originalImage, float scale)
  89.     {
  90.         _originalImage = originalImage;
  91.         PrepareScaledImage(scale);
  92.         RenderStretchedImage();
  93.         return _workingImage;
  94.     }
  95.  
  96.     private void PrepareScaledImage(float scale)
  97.     {
  98.         int width = (int)(_originalImage.Width * scale);
  99.         int height = (int)(_originalImage.Height * scale);
  100.         _workingImage = new Bitmap(width, height);
  101.     }
  102.  
  103.     public Image ScaleImage(Image originalImage, float widthScale, float heightScale)
  104.     {
  105.         _originalImage = originalImage;
  106.         PrepareScaledImage(widthScale, heightScale);
  107.         RenderStretchedImage();
  108.         return _workingImage;
  109.     }
  110.  
  111.     private void PrepareScaledImage(float widthScale, float heightScale)
  112.     {
  113.         int width = (int)(_originalImage.Width * widthScale);
  114.         int height = (int)(_originalImage.Height * heightScale);
  115.         _workingImage = new Bitmap(width, height);
  116.     }
  117. }

One of the first thing you might notice is that the code has grown from 75 lines to 117. This is just a price you have to pay, but I believe that readability should almost always outweigh brevity.

In the first method, I violate the principle of function size, but that is, unfortunately, necessary when using switch statements. You'll notice that the actions performed in the function is relegated to other functions, which is the main way this technique requires code to be structured.

The functions are topologically sorted, such that a given function is directly followed by the functions used in its body, which again are followed by their own dependencies, etc. Think of it as a stack that, as you read through the file, pushes the most relevant functions to the top as needed. Thus, the first ResizeImage function is followed by the function PrepareImage, which is its first dependency. If this function had had any dependencies, they would have followed immediately after before continuing with the dependencies of ResizeImage.

Apart from creating a very literate piece of code, the transformation also revealed a few cases of repeated code which was turned into reusable functions. Where before I might not have bother with looking for such similarities in the code, it becomes more obvious when splitting everything into its essential parts.

When I read about this approach, it struct me as a very clever way to structure your code, but there are a few caveats that I'm not too sure about, yet. I find that the resulting code can be a bit overwhelming to look at as its structure has become more homogeneous and, as mentioned above, there are just more lines of code to read. It might help, though, if the IDE would highlight the public functions, so you could quickly navigate to the part of the code you need. Regardless of these issues, I find the approach very interesting and I'm looking forward to seeing how well it works for me.

If you found the technique interesting, I'm sure you'll love the book, as it's filled with little nuggets like this.

Comments

Jan Bothma

on 15-04-2011 at 13:35

I haven't read clean code myself, but I tend to side with the style you show in the second listing.While more functions means more function names and their meanings to keep track of (this grows quickly!), that's easily dealt with by features like Eclipse displaying a function body when you hover over it's name. I guess that's one way to answer your comment about the IDE highlighting the relevant parts.