Clean Code – Part 2

It’s time for the second installment of ‘Clean Code’. If you missed the first one you can find it here:

Principle of Proximity

This principle is based on ‘tell me what I need to know - when I need to know‘. I see new developers often caught up adding many unrelated variables to the top of their methods.

Adding properties and backing variables to the top of a class definition is fine. However, when it comes to method implementations, following the same approach can lead to readability issues.

Take for instance the code below. It contains a method which generates an invoice document and then emails it to the customer:

var smtpHost = AppSettingsHelper.GetSmtpHost();
var from = AppSettingsHelper.GetFromMailAddress();
var subject = "Your bill is ready.";
var to = AppSettingsHelper.GetToMailAddress();
var billableItems = _billingService.GetBillForUser(userId);

//
// long lines of code here to generate a pdf file with 
// the invoice details...
//
// var invoice = ...
//

// send email with invoice as attachment
var ms = new MemoryStream(Encoding.GetBytes(invoice));
var attachment = new Attachment(ms, "Invoice.xml");  

var body = $"Hi, here is your invoice blah blah... "

using (SmtpClient client = new SmtpClient(smtpHost)
{
  var message = new MailMessage(from, to, subject, body);   
  message.Attachments.Add(attachment);

  client.Send(message);
}

Imagine yourself or another developer coming back to this code six months later. There are a lot of variables to comprehend, and most of those are largely unrelated to the proceeding code.

Here’s a better structure:

var billableItems = _billingService.GetBillForUser(userId);

//
// long lines of code here to generate a pdf file with 
// the invoice details...
//
// var invoice = ...
//

// send email with invoice as attachment

var smtpHost = AppSettingsHelper.GetSmtpHost();
var from = AppSettingsHelper.GetFromMailAddress();
var subject = "Your bill is ready.";
var to = AppSettingsHelper.GetToMailAddress();

var body = $"Hi, here is your invoice blah blah... "

using (SmtpClient client = new SmtpClient(smtpHost)
{
  var message = new MailMessage(from, to, subject, body);  
  var ms = new MemoryStream(Encoding.GetBytes(invoice));
  var attachment = new Attachment(ms, "Invoice.xml");  

  message.Attachments.Add(attachment);
  client.Send(message);
}

Much better. Now when reading the code you only need to comprehend the variables relating to the proceeding code. For example, the email related variables are kept around the email creation logic.

Refactoring is good – just don’t overdo it.

The readability of the sample code above can be further improved by moving significant portions of work to their own methods. This also helps when debugging by making it much easier to pinpoint where errors may be occurring. New developers benefit too, by helping them understand “what the code is trying to do” without being bogged down by noise in “how” it is doing it.

try
{
  var user = GetUser(userId);

  var billableItems = _billingService.GetBillForUser(userId);

  var invoice = GenerateInvoice(user, billableItems);

  EmailInvoice(user, invoice);
}
catch(Exception)
{
  // handle exception
}

When refactored correctly, code should be “easy” to read and not require so many comments. Note that overzealous refactoring can also cause harm. Extracting code to methods excessively causes inefficiency as the developer scrambles through the codebase to find bits of code.

Enums in place of comments

As developers, we often fall into the trap of using raw integer values directly in code. I’ve mentioned before about storing such values in a database or configuration file, though for data values that are unlikely to change this could be overkill. In those cases, using integers in the code is the preferred choice.

For example, let us imagine a ‘status’ method on an entity…

device.setStatus(1);    // 0 = disabled, 1 = enabled, 2 = hybrid

Using a boolean value is not sufficient in this example as there three possible states. In this case, using integers makes sense, but this causes two problems with readability:

1. A reliance on comments for describing values. Not everyone writes helpful comments.
2. Duplication of comments. Comments describing values in this way are likely to be copied and pasted across the codebase where similar code exists. Even worse, changes made later to the values may invalidate the comment description altogether.

So what’s the answer?... Create an Enum:

enum DeviceStatus {
    Disabled = 1,
    Enabled = 2,
    Hybrid = 3
}

Then we can do this:

device.setStatus(DeviceStatus.Enabled);

The code has been improved by creating an Enum type DeviceStatus. It has three possible values that can be extended easily. Each value in the Enum has an underlying integer value. Thus in programming languages similar to C# they can be easily used in places instead hard-coding integer values.

The intent of the code is more clear and reduces the need for comments.

Comments

When I first started programming many years ago, I fell foul of over-commenting my code. It's a trap all new developers fall into:

// declare integer variable
int count = 0; // initialize to zero;

// declare string
string name;

There is a natural tendency to do this when you're new because you are learning. Commenting like this is akin to note-taking. It might be useful short term but in the long term it just adds unnecessary noise.

I prefer to comment only in a few instances.

Examples include:

• code logic that is unable to explain itself adequately.
• interfaces - using a <summary> boilerplate on interface methods.

// Customer's FTP server has intermittent network connection issues. 
// Retry a few times before giving up.

bool success = false;
int failCount = 0;

while(!success)
{
  success = await _ftp.Upload(file);

  if(success) break;

  failCount++;

  if(failCount > 3) break;

  Thread.Sleep(2000);
}

If you aren't confused by what a piece of code is doing, but rather why it's doing it at that moment, then a comment should be added. In the example above, the code is telling us a file is being uploaded over FTP - but it may not be obvious why the while loop is being used.

The comment is valuable here is it explains the reasoning as to why the logic has been implemented in this way.

Thank you to all who have followed and liked my posts - I will be back with more soon! 🤓