Clean Code
- Nick
- 19 March 2022
Clean Code
Best Practices
Clean Code refers to the art of writing beautiful code and it’s the kind of code that is clean, maintainable, reveals the intent, reduces the guess work, and reads like a story. That’s how the code should be.
To understand it better we are going to look at several examples of poorly written code which are called code smells. For each example we will see what is wrong with the code and how to fix it.
The examples that follow are in C#.
Poor Names
Poor names are one of the most common code smells. We have 5 categories:
-
Mysterious names
int nd; string n;Problem: These identifiers do not reveal their intent and you need to check the code to understand its meaning.
Solution: Your code should be clean that reveals intent, so the reader does not have to see somewhere else to understand what is happening.
int numberOfDays; string name; -
Meaningless names
void BeginCheckFunctionality_StoreClientSideCheckboxArray();Problem: From the name you cannot understand what this method does. You must look at the implementation. Typically, a method like that is more than 100 lines of code and in my experience that is the reason that has this meaningless name. When your function has more than 10 lines of code it is most likely doing too many things and coming with clean intention name is not easy.
Solution: Keep your method short so the method is doing only one thing and it easy to come up with meaningful name.
-
Encodings in names
int iMaxRequest;Problem: This is a Hungarian Notation. This was very popular in the past as it revealed the data type of a variable. We have very good IDE-s that reveal the data types very easy. There is no need to pollute identifiers with their data type.
Solution
int maxRequests; -
Ambiguous names
bool MultiSelect() {}Problem: What does this method do here? Is it telling me if multiple items are selected? Or is it to select multiple items? I cannot tell. I must look inside the method.
Solution:
bool AreMultipleItemsSelected() {} -
Noisy names
List<Customer> listOfApprovedCustomers;Problem: The name of the variable gets bigger and the word “list” in front of the variable does not add value.
Solution:
List<Customer> ApprovedCustomers;
Poor Naming Convention
Developers coming from different background have adopted different naming conventions.
Customer GetCustomer(int _id);
Customer saveCustomer(Customer Customer);
private int customerId;
Problem: The examples show that different naming conventions are used on the same project.
Solution: Try to use the default naming convention that a framework has. This way the code will be cleaner, and it looks like it’s written by one person as opposed to 50. In case of c# the correct naming should be:
Customer GetCustomer(int id);
Customer SaveCustomer(Customer customer);
private int _customerId;
Poor Method Signatures
Another very common code smell is poor method signatures.
void Parse(int command);
Problem: The method is called Parse. Parse means accepting a string as parameter and convert it to a different object while in this case it’s accepts integer and does not return anything.
Solution: When writing a method triple check the return type, method name and its parameters. Make sure all this 3 elements are logically related. A good example for the above example would be
int Parse(string command);
Long Parameter List
The more parameters a method has, the harder it gets to understand its intention.
void checkNotifications(null, 1, true, false, DateTime.now)
Problem: What the above arguments represent? What is null? What is true? We cannot tell without having a look at the implementation of that method. It is hard to understand and hard to use .
Solution: Limit the number of parameters to 3. When you have 3 parameters you are pretty much on the edge. More than 3 parameters is usually a code smell and you need to get rid of unnecessary parameters or encapsulate the logically related ones into a class.
Variable Declarations on the Top
In the old days in 70s and 80s it was common to declare variables on top of the method and that because early compilers were not able to understand a variable that was declared in the middle of a method. But these days we do not have that problem anymore. We should declare our variables close to their usage. Let's consider the following example:
public decimal CalcGross(decimal rate, decimal hours)
{
decimal overtimeHours = 0;
decimal regularHours = 0;
decimal regularPay = 0;
decimal overtimePay = 0;
decimal grossPay = 0;
if (_payFrequency == PayFrequency.Fortnightly)
{
if (hours > 80)
{
overtimeHours = hours - 80;
regularHours = 80;
}
else
regularHours = hours;
}
else if (_payFrequency == PayFrequency.Weekly)
{
if (hours > 40)
{
overtimeHours = hours - 40;
regularHours = 40;
}
else
regularHours = hours;
}
if (overtimeHours > 0m)
{
overtimePay += (rate * 1.5m) * overtimeHours;
}
regularPay = (regularHours * rate);
grossPay = regularPay + overtimePay;
return grossPay;
}
Problem: If you see the variables regular pay, regularPay, grossPay, they are used on the end of the method but declared in the beginning.
Solution: Declare variables close to their usage.
public decimal CalcGross(decimal rate, decimal hours)
{
decimal overtimeHours = 0;
decimal regularHours = 0;
if (_payFrequency == PayFrequency.Fortnightly)
{
if (hours > 80)
{
overtimeHours = hours - 80;
regularHours = 80;
}
else
regularHours = hours;
}
else if (_payFrequency == PayFrequency.Weekly)
{
if (hours > 40)
{
overtimeHours = hours - 40;
regularHours = 40;
}
else
regularHours = hours;
}
decimal overtimePay = 0;
if (overtimeHours > 0m)
{
overtimePay += (rate * 1.5m) * overtimeHours;
}
decimal regularPay = (regularHours * rate);
decimal grossPay = regularPay + overtimePay;
return grossPay;
}
Magic Numbers
public class MagicNumbers
{
public void ApproveDocument(int status)
{
if(status == 1)
{
//..
}
else if(status == 2)
{
//..
}
}
}
Problem: The magic number here is 1, because for me as a reader i have no idea what number 1 represents. This kind of magic numbers make code hard to read, hard to understand and hard to change.
Solution: One solution is introduce a variable with a proper name to represent the information:
public class MagicNumbers
{
public void ApproveDocument(int status)
{
const int Draft = 1;
const int Published = 2;
if(status == Draft)
{
//..
}
else if(status == Published)
{
//..
}
}
}
This approach is ok when you use the Magic Numbers in one method. If you want to use them in multiple methods, consider creating an Enum:
public enum DocumentStatus
{
Draft = 1,
Published = 2
}
public class MagicNumbers
{
public void ApproveDocument(DocumentStatus status)
{
if(status == DocumentStatus.Draft)
{
//..
}
else if(status == DocumentStatus.Published)
{
//..
}
}
}
Nested Conditionals
This kind of code smell is often seen amongst junior and sometimes intermediate developers.
If(a)
{
if(b)
{
if(c)
{
}
else
{
}
}
}
Problem: Hard to read, hard to change and hard to test. The more conditionals, the more execution paths we have which means we need a variation combination of values to test the method.
Solution 1: Use Ternary Operator
Imagine you have a pattern like this in your code.
if(customer.TotalOrders > 50)
discount = 0.1f;
else
discount = 0.01f;
We can write the above nested conditions using the ternary operator.
discount = (customer.TotalOrders > 50) ? 0.1f : 0.01f;
Solution 2: Simplify true/false
if(customer.TotalOrders > 50)
isGoldCustomer = true;
else
isGoldCustomer = false;
We can simplify to an expression:
isGoldCustomer = customer.TotalOrders > 50
Solution 3: Combine
if(a)
{
if(b)
{
statement
}
}
We can use the logical operators to rewrite in simpler way:
if(a && b)
{
statement
}
Solution 4: Early Exit
if(a)
{
if(b)
{
statement
}
}
We can use the guard statements:
if(!a || !b)
{
return
}
statement
Solution 5: Swap Orders
if(a)
{
if(b)
{
isValid = true;
}
}
if(c)
{
if(b)
{
isValid = true;
}
}
In both blocks isValid is set to true only if b is true. We can swap the orders and apply the above techniques to simplify it to :
isValid = b & (a || v);
Switch Statements
Switch statements are popular amongst programmers who are not familiar with object oriented programming. So they are used to doing things in a procedural way, which means calling functions and functions and functions and passing argument around. In object oriented programming we thing of objects as opposed to functions. Objects are about data and behaviour. Let's take a look some switch statements.
public class Customer
{
public CustomerType Type { get; set; }
}
public enum CustomerType
{
PayAsYouGo = 1,
Unlimited
}
public class MonthlyUsage
{
public Customer Customer { get; set; }
public int CallMinutes { get; set; }
public int SmsCount { get; set; }
}
public class MonthlyStatement
{
public float CallCost { get; set; }
public float SmsCost { get; set; }
public float TotalCost { get; set; }
public void Generate(MonthlyUsage usage)
{
switch (usage.Customer.Type)
{
case CustomerType.PayAsYouGo:
CallCost = 0.12f * usage.CallMinutes;
SmsCost = 0.12f * usage.SmsCount;
TotalCost = CallCost + SmsCost;
break;
case CustomerType.Unlimited:
TotalCost = 54.90f;
break;
default:
throw new NotSupportedException("The current customer type is not supported");
}
}
}
Problem: There are 3 problems with the code. The first problem is that if we want to add a new switch case we have to modify this method. This means that this class has to be recompiled and every other class of the application that is dependent on this class has to be recompiled and this cycle goes all the way up to the application root. We want to design our software to be open for extensions and close to modification. This means we want to change the behavior of our Software by adding new classes as opposed to change the existing, because every time we change an existing code we may break things along the way.
The second problem is that if we add a new customer type this method grows up larger and larger.
The third problem is that when we have switch statements based on the type of the object, it is quite possible that this switch statement is found all over the application for different reasons. Sometimes we want to calculate cost based on the type of customer. Sometimes we want to give different promotions or vouchers and as you see we will have different rules and all are based on the type of customer.
Solution: The Solution is polymorphism. In this case we will not have one customer class but a hierarchy of customers. We will have a parent class and it's derivatives.
public class MonthlyUsage
{
public int CallMinutes { get; set; }
public int SmsCount { get; set; }
}
public class MonthlyStatement
{
public float CallCost { get; set; }
public float SmsCost { get; set; }
public float TotalCost { get; set; }
}
public abstract class Customer
{
public abstract MonthlyStatement GenerateStatement(MonthlyUsage usage)
}
public class PayAsYouGoCustomer : Customer
{
public MonthlyStatement GenerateStatement(MonthlyUsage usage)
{
var statement = new MonthlyStatement();
statement.CallCost = 0.12f * usage.CallMinutes;
statement.SmsCost = 0.12f * usage.SmsCount;
statement.TotalCost = statement.CallCost + statement.SmsCost;
return statement;
}
}
public class UnlimitedCustomer : Customer
{
public MonthlyStatement GenerateStatement(MonthlyUsage usage)
{
var statement = new MonthlyStatement();
statement.TotalCost = 54.90f;
return statement;
}
}
This is how to break down a fat switch block into polymorphic dispatch.
Duplicated Code
This is a code smell that we should avoid at all time, because if you need to change it, you need to make that change in multiple places. Plus duplicated code makes code noisy and hard to understand. There is a principle called DRY(Don't Repeat Yourself)
public class DuplicatedCode
{
public void AdmitGuest(string name, string admissionDateTime)
{
// Some logic
// ...
int time;
int hours = 0;
int minutes = 0;
if (!string.IsNullOrWhiteSpace(admissionDateTime))
{
if (int.TryParse(admissionDateTime.Replace(":", ""), out time))
{
hours = time / 100;
minutes = time % 100;
}
else
{
throw new ArgumentException("admissionDateTime");
}
}
else
throw new ArgumentNullException("admissionDateTime");
// Some more logic
// ...
if (hours < 10)
{
}
}
public void UpdateAdmission(int admissionId, string name, string admissionDateTime)
{
// Some logic
// ...
int time;
int hours = 0;
int minutes = 0;
if (!string.IsNullOrWhiteSpace(admissionDateTime))
{
if (int.TryParse(admissionDateTime.Replace(":", ""), out time))
{
hours = time / 100;
minutes = time % 100;
}
else
{
throw new ArgumentException("admissionDateTime");
}
}
else
throw new ArgumentNullException("admissionDateTime");
// Some more logic
// ...
if (hours < 10)
{
}
}
}
Problem: The parsing of the admission date is repeat in both methods
Solution:
public class Time
{
public int Hours { get; set;}
public int Minutes { get; set;}
public static Time Parse(string str)
{
if (!string.IsNullOrWhiteSpace(str))
{
int time;
if (int.TryParse(str.Replace(":", ""), out time))
{
Hours = time / 100;
Minutes = time % 100;
}
else
{
throw new ArgumentException("str");
}
}
else
throw new ArgumentNullException("str");
}
}
public class DuplicatedCode
{
public void AdmitGuest(string name, string admissionDateTime)
{
// Some logic
// ...
var time = Time.Parse(admissionDateTime);
// Some more logic
// ...
if (time.Hours < 10)
{
}
}
public void UpdateAdmission(int admissionId, string name, string admissionDateTime)
{
// Some logic
// ...
var time = Time.Parse(admissionDateTime);
if (time.Hours < 10)
{
}
}
}
Next time you see duplicated code remember the DRY principle.
Comments
Are comments code smells? In most cases they are. Let's see some examples
-
Stating the Obvious
// Returns list of customers in a country. public List<Customer> GetCustomers(int countryCode) { }Problem: The comment does not add any value. The name of the method is pretty clear enough. A comment like that just creates noise in the code.
-
Version History
// Prior to v1.3 if(isActivce){}Problem: The version number does not add value. These kind of comments are completely useless. These days we use version control systems. If you want you can go back to history to check the version information.
-
Clarify the Code
var pf = 10; //Pay FrequencyProblem: I have no idea of the meaning of pf variable1 until i read the comment.
Solution: We can name the variable pay frequency and remove the redundant comment.
var payFrequency = 10; -
Dead Code
// public class WorkScheduler // { // }Problem: Why comment a code that is not used? If you do not need it just delete it. You can always pull back from the code repository.
As Uncle Boob said: Do not write comments, re-write your code! Don't explain "whats"(the obvious).
Comments are helpful when explaining the "whys" and "hows".
Long Methods
Long Methods are the most popular code smells. A long method is one that is more than 10 lines of code. On a scientific level we as humans can keep up to seven things at a time in our conscious mind, which means when you read a method 100 lines of code you have to keep track of a lot of things from the top of the method to the bottom.
public partial class Download : System.Web.UI.Page
{
protected void Page_Load(object sender, EventArgs e)
{
System.IO.MemoryStream ms = CreateMemoryFile();
byte[] byteArray = ms.ToArray();
ms.Flush();
ms.Close();
Response.Clear();
Response.ClearContent();
Response.ClearHeaders();
Response.Cookies.Clear();
Response.Cache.SetCacheability(HttpCacheability.Private);
Response.CacheControl = "private";
Response.Charset = System.Text.UTF8Encoding.UTF8.WebName;
Response.ContentEncoding = System.Text.UTF8Encoding.UTF8;
Response.AppendHeader("Pragma", "cache");
Response.AppendHeader("Expires", "60");
Response.ContentType = "text/comma-separated-values";
Response.AddHeader("Content-Disposition", "attachment; filename=FooFoo.csv");
Response.AddHeader("Content-Length", byteArray.Length.ToString());
Response.BinaryWrite(byteArray);
}
public System.IO.MemoryStream CreateMemoryFile()
{
MemoryStream ReturnStream = new MemoryStream();
try
{
string strConn = ConfigurationManager.ConnectionStrings["FooFooConnectionString"].ToString();
SqlConnection conn = new SqlConnection(strConn);
SqlDataAdapter da = new SqlDataAdapter("SELECT * FROM [FooFoo] ORDER BY id ASC", conn);
DataSet ds = new DataSet();
da.Fill(ds, "FooFoo");
DataTable dt = ds.Tables["FooFoo"];
//Create a streamwriter to write to the memory stream
StreamWriter sw = new StreamWriter(ReturnStream);
int iColCount = dt.Columns.Count;
for (int i = 0; i < iColCount; i++)
{
sw.Write(dt.Columns[i]);
if (i < iColCount - 1)
{
sw.Write(",");
}
}
sw.WriteLine();
int intRows = dt.Rows.Count;
// Now write all the rows.
foreach (DataRow dr in dt.Rows)
{
for (int i = 0; i < iColCount; i++)
{
if (!Convert.IsDBNull(dr[i]))
{
string str = String.Format("\"{0:c}\"", dr[i].ToString()).Replace("\r\n", " ");
sw.Write(str);
}
else
{
sw.Write("");
}
if (i < iColCount - 1)
{
sw.Write(",");
}
}
sw.WriteLine();
}
sw.Flush();
sw.Close();
}
catch (Exception Ex)
{
throw Ex;
}
return ReturnStream;
}
}
Problem: The CreateMemoryFile() method is more than 60 lines of code. This makes it hard to read, hard understand, hard to change.
Solution: We want to break it down to a set of smaller methods around 10 lines of code. One criteria for splitting the larg method to smaller ones is cohesion. Cohesion says that things that are related should be together and things that are not related should be not together.
public class TableReader
{
public DataTable GetDataTable()
{
var connection = new SqlConnection(ConfigurationManager.ConnectionStrings["FooFooConnectionString"].ToString());
var dataAdapter = new SqlDataAdapter("SELECT * FROM [FooFoo] ORDER BY id ASC", connection);
var dataSet = new DataSet();
dataAdapter.Fill(dataSet, "FooFoo");
return dataSet.Tables["FooFoo"];
}
}
public class DataTableToCsvMapper
{
private void WriteColumnNames(DataTable dt, SteamWriter sw)
{
for (int i = 0; i < dt.Columns.Count; i++)
{
sw.Write(dt.Columns[i]);
if (i < dt.Columns.Count - 1)
{
sw.Write(",");
}
}
sw.WriteLine();
}
private void WriteRow((DataTable dt, SteamWriter sw, DataRow dr)
{
for (int i = 0; i < dt.Columns.Count; i++)
{
if (!Convert.IsDBNull(dr[i]))
{
string str = String.Format("\"{0:c}\"", dr[i].ToString()).Replace("\r\n", " ");
sw.Write(str);
}
else
{
sw.Write("");
}
if (i < iColCount - 1)
{
sw.Write(",");
}
}
}
private void WriteRows(DataTable dt, SteamWriter sw)
{
foreach (DataRow dr in dt.Rows)
{
WriteRow(dt, sw, dr);
sw.WriteLine();
}
}
public System.IO.MemoryStream Map(DataTable dataTable)
{
var ReturnStream = new MemoryStream();
var sw = new StreamWriter(ReturnStream);
WriteColumnNames(dataTable, sw);
WriteRows(dt, sw);
sw.Flush();
sw.Close();
return ReturnStream;
}
}
public partial class Download : System.Web.UI.Page
{
protected void Page_Load(object sender, EventArgs e)
{
ClearResponse();
SetCacheability();
WriteContentToResponse(GetVsv());
}
private byte[] GetVsv()
{
var tableReader = new TableReader();
var dataTable = tableReader.GetDataTable();
var mapper = new DataTableToCsvMapper();
var memoryStream = mapper.map(dataTable);
byte[] byteArray = memoryStream.ToArray();
memoryStream.Flush();
memoryStream.Close();
return memoryStream;
}
private void ClearResponse()
{
Response.Clear();
Response.ClearContent();
Response.ClearHeaders();
Response.Cookies.Clear();
}
private void SetCacheability()
{
Response.Cache.SetCacheability(HttpCacheability.Private);
Response.CacheControl = "private";
Response.AppendHeader("Pragma", "cache");
Response.AppendHeader("Expires", "60");
}
private void WriteContentToResponse(byte[] byteArray)
{
Response.Charset = System.Text.UTF8Encoding.UTF8.WebName;
Response.ContentEncoding = System.Text.UTF8Encoding.UTF8;
Response.ContentType = "text/comma-separated-values";
Response.AddHeader("Content-Disposition", "attachment; filename=FooFoo.csv");
Response.AddHeader("Content-Length", byteArray.Length.ToString());
Response.BinaryWrite(byteArray);
}
}
Methods should be short(less than 10 lines). Methods should do only one thing(Single Responsibility Principle).
