What I have learned about clean coding

#codenewbie #codequality #cleancode #codereview

I have been making my way through Uncle Bob's (Robert C. Martin) Clean Code: A Handbook of Agile Software Craftsmanship over the year. I thought I'd share some things I learned from the book.

1. Why is clean code important?

Poor quality code can happen for many reasons: lack of time, technical limitations, large workload, not enough programmers on the team and so on. Having clean code is important because bad code can impede the velocity of the team, reduce morale/motivation, cause a large amount of technical debt, or hide lurking bugs, resulting in an increase in project cost or delays to release. Sometimes a poor codebase gets so difficult to work in that the team want a complete redesign or developers' morale drop.

We've all had days where we've implemented a bit of logic and we know we can do it better but we are pushed for time or just have gotten to the point where we need to move on, and we live with what we have written. Or we have a looked at a code review and thought "oof there's a slightly cleaner way to do this", but we approve the PR and only highlight it to the developer as something to keep in mind in the future. Granted sometimes that can be efficient in the short term but what's the likelihood that they will remember that improvement in a few weeks time if they don't implement the code change whilst they are still in the context of the issue? (Probably low). And by approving that code, you have already degraded your codebase and caused technical debt, which will require more work in the future.
The responsibility of code quality ultimately comes down to us. The developers, the people who write and review the stuff. If you are not happy with the quality of your codebase then it is up to you and the other developers to make the changes to improve it.

So how can we improve? The tools and technologies we use are always changing, however there are development principles and practises which have been created by experienced individuals and we can use these as guidelines. That's not say they will never change, of course we need to update them as development evolves but that comes with experience. By adopting and sharing development principles and practises with others we can improve the standard of our code to achieve "clean code nirvana" or at least something close to it.

2. What practises have I learned from the the Clean code book?

"Clarity is King." - Robert C. Martin

In development teams, a common cause of issues or delays is down to lack of clarity, whether that be in requirements, architecture decisions, release dates, team structure… the list can go on. In terms of codebase - clarity is required in so many different areas. Naming conventions, size and responsibilities of classes and functions, reusable code, code review comments, formatting, error handling, unit/component tests, code structure and design.

Naming conventions was an interesting point for me because I didn't quite realise how wrong you could go with names. Names should be intention revealing, meaningful, and clear. A common issue we have with function or class names is that they are not searchable, I.e. When search for a class called price we get: PricePoint.cs, Price.cs, PriceData.cs - "which one do we actually want? We have no idea". Agreeing on naming conventions within the team so everyone is aware of the format you will take before the project starts can combat this - this can be documented in service contracts or domain model design wikis and updated as time goes on.

"Functions should do one thing." - Robert C. Martin

Naming conventions leads us nicely onto to abstraction (there is a good blog post here which covers the definition and different examples of abstraction), and single responsibility principle. We all know and love SRP - functions and classes should have a single job to do, this makes it easier for us to identify what will be affected if any if any changes are made to the function or classes. But how can we determine what 'one thing is'? We should be able describe a function or class in a small statement, if we take the following example:

private static int AddTwoNumbers(int firstNumber, int secondNumber)
{
    int total;
    total = firstNumber + secondNumber;
    return total;
}
// AddTwoNumbers adds two integer parameters and returns the result.

The function is doing only the steps described and the name of the function is related to the one thing it is doing (the function has one level of abstraction). We also have one section in this function which encapsulates the steps taken to add the numbers and return the result, then we can say this function has a single responsibility.

If we take the following example…

private static int AddTwoNumbers(decimal firstNumber, decimal secondNumber)
{
    int firstNumberInt = Convert.ToInt32(firstNumber);
    int secondNumberInt = Convert.ToInt32(secondNumber);
    int total;
    total = firstNumber + secondNumber;
    return total;
}

we can clearly see there are two sections in the function, one which does a data type conversion from decimal to integer and the second part which does the addition. It's also causing us to have poorly named variables in the function (firstNumberInt & secondNumberInt). This function clearly does not have a single responsibility and it's name does not relate completely to what is being done.

"A function that handles errors should do nothing else." - Robert C. Martin

Error handling is an important part of having good clean code. There is nothing more unhelpful than getting a blanket failure message of "object reference is not set to an instance of an object" somewhere in your code. Let's take some test code as an example.

private void CheckNumberThreeIsShown()
{
    IWebElement numberElement = FindElement(By.Id("test-number"));
    int number = int.Parse(numberElement.Text);
    Assert.AreEqual(3, number, $"Number should be 3 but 
       is actually {number}.";
} 
// Finds element, gets text from element, converts into int and assert number is 3 as expected.

The above code makes sense, but what happens if the text of the numberElement is null or not a number? We encounter a SystemFormatException when we assert instead of our useful error message, which would require us to debug through the code to find the failure point.

private void CheckNumberThreeIsShown()
{
    IWebElement numberElement = GetElement(By.Id("test-number"));

    string numberText = GetElementText(numberElement);
    int number = ConvertStringToInt(numberText);
    Assert.AreEqual(3, number, $"Number should be 3 but 
       is actually {number}.";
}

private IWebElement GetElement(By locator)
{
   try
   {
      return _driver.FindElement(element);
   }
   catch(NoSuchElementException e)
   {
      throw new WebDriverException($"{element} 
         could not be located" + e);
   }
} 
// Gets an element, throws an error if unsuccessful.

private string GetElementText(IWebElement element)
{
   return element.Text;
} 
// Gets element text, throws error if unsuccessful.

private int ConvertStringToInt(string text)
{
   try         
   {             
      return int.Parse(text);         
   }         
   catch (FormatException e)         
   {             
      Console.WriteLine($"Unable to parse '{text}' into integer");         
   }  
} 

// Tries to convert a string to integer, prints message if unsuccessful.

If we look at the example above, we can see each function handles errors and throws a descriptive message for the user. If there is a failure during this test, we will be able to clearly see where the failure point is and why without having to debug through the code.

"Comments do not make up for bad code."- Robert C. Martin

Comments can be cluttering in the code, but are useful in some circumstances. When using comments they should be concise and provide value. i.e. the first comment below is redundant and noisy but the second comment is useful to note.

private static IWebElement GetElement(By locator)
{
   try
   {
      return _driver.FindElement(element);
   } // returns the element.
   catch(NoSuchElementException e)
   {
      throw new WebDriverException($"{element} 
         could not be located" + e);
   }
}
//To do: create function to wait and retrieve IWebElement using locator.

The "to do" comment is a common way to remind yourself (or others) there is extra work to do, if you cannot complete that work in time. They can often be comments where you would like to take time to refactor or there was extra functionality you could have added in but it is a requirement covered by a different user story. When adding to do comments it is helpful to also write the story or task number which will cover this work so it can be clearly tracked in the backlog.
Finally, FORMATTING. Formatting our code is essential, it provides clarity (which equals king) to our code. Key takeaways are to ensure the team has the same formatting rules and that these are checked in PRs. Dependent or related functions should be near each other where possible so other developers can follow the code easily and class maintains readability.

3. Where can we have clean code?

In short, everywhere. The above comments are not strictly just for development code but also applies to test code - unit, component, or functional tests. The quality of test code should be maintained to the same standards as production code. Having poor quality test code can lead to unreliable tests, incorrect tests, and can become very difficult to maintain as more tests are added.

4. How can we make our code cleaner?

It's really easy to have a list of best practises and say you'll abide by them until the end of time, but time pressure is REAL and sometimes you get pushed to meet that deadline. My biggest question is how can we stop ourselves falling into bad habits?

A way to practise clean coding is to identify your weaknesses and strengths when it comes to maintaining quality of code. Narrowing down the cause of the weaknesses and how you can improve on them. But also, see where you do well and if you can impart that knowledge onto other members of your team.
A typical habit is not being critical enough in code reviews when we are under a time constraint. When reviewing a colleague's code or our own, even if the team have time constraints, we should review every line and see if it abides by coding principles, but when there is a time pressure we tend to put changes into technical debt tasks or do not flag the issues. What's the cause? Usually our minds are already on the next story we need to work on and the "if the code I have already written works why not move on" mentality kicks in. How can we work on this? Estimate extra time (30mins?) for self code reviews, so we can take time to review it before we raise a PR for our peers to review or if reviewing a colleagues ensure you have a task estimated which sufficient time that allows you to give a thorough review. Get into the mind frame of 'is this good quality code' and remove distractions when reviewing (don't try to multitask).

Different developers also have different conventions, it may be useful to define your development team best practises (naming conventions, code structure and design, error handling, unit/component test definitions) when a project starts and have a shared wiki within the team where this is outlined - this would also double up as training documentation for new members of the team.

Most people do not write clean code from day one, it takes practise. In the beginning it's easier to write something that works and then go back in a refine what you have written, splitting functions into smaller ones where possible and renaming variables/functions as you give them a single responsibility. Refactoring is always your friend and although it sometimes seems like a mammoth task, don't be afraid to do it. Eventually, the practises will come naturally and you'll be able to refactor things as you go rather than in one big bulk.

Finally, if you do find yourself or team members falling into bad habits or feeling extreme time constraints, just communicate with your team. Retrospectives are good places to do this as they are open forums where you can discuss things you'd like to implement within the team. Sometimes people forget that the documentation is there so if a point like this is brought up it can be a helpful reminder to other members of the team, it can also make visible whether the time constraints are so severe that you actually need more team members.
If you've made it this far, thank you for reading. I hope it has been useful and do check out the links below if you would like more information.

The book contains a lot of useful information. I've summarised some key things I've taken away but it is not a full summary of what the book has to offer. There are some topics I have not covered in this particular blog (emergents, concurrency, code smells etc) as I want to research them further. Hopefully once I have done so I can write a follow up blog.