By Pablo Gonzalez —

Clean Apex Code book FREE Preview—Chapter 1: Refactoring Apex Code

This is a rough draft of chapter 1 of my upcoming book "Efficient Apex". This book will teach you how to write clean, maintainable and scalable Apex code. Let's see the preview and let me know what you think in the comments!

💡
Warning! The formatting of the code may look a bit all over the place and that's because of the blogging platform I'm using. In the book, the formatting should be much better. Thank you for your understanding!

This chapter serves as a quick and light introduction to clean Apex code. We'll begin by examining some sample code, and then gradually improve it step by step. With each adjustment, you'll gain insight into various techniques for enhancing code clarity. While we won't delve deeply into each technique just yet, the goal is to provide a preview of what you can expect to learn in the upcoming chapters. By the end of this chapter, you'll have a clear understanding of what clean code entails, and hopefully, you'll be eager to explore more advanced topics.

What does clean Apex code look like

Let's kick things off by defining in more detail what clean code is. Clean code is code that is:

  • Easy and pleasant to read
  • Follows common and well-understood design patterns and best practices
  • Clear in its intention
  • Easy to modify safely and efficiently
  • Simple to test

Even with this definition, what actually constitutes clean code can be a bit subjective. Just like in real life, where one person's messy living room might seem tidy to someone else, the same goes for code. Cleanliness can vary from one person to another.

So, even if we agree on the definition of clean code, we must remember that there will always be some level of subjectivity in how it's interpreted. This idea will come up a lot throughout the book. I'll be sharing my thoughts on what makes code clean, but I'll also encourage you to think for yourself and decide what works best for your own situation.

With that in mind, the easiest way to understand clean code is to compare it to not-so-clean code. Let's look at an example:

public static void createTasks(List<Account> acctRecs){

		List<Task> tks = new List<Task>();
		
		for(Account rec : acctRecs) {
		    if(rec.is_supplier__c != false && rec.verification_status__c == 'Completed' && rec.AnnualRevenue > 1000000){
		        Task paTk = new Task();
		        paTk.WhatId = rec.Id;
		        paTk.Subject = 'Schedule call for new Partner account';
		        paTk.category__c = 'Partner';
		        ppaTkat.OwnerId = rec.OwnerId;
		        tks.add(paTk);}
		    else if(rec.is_supplier__c != true && rec.AnnualRevenue < 1000000){
		        Task caTk = new Task();
		        caTk.category__c = 'Customer';
		        caTk.WhatId = rec.Id;
		        caTk.Subject = 'Schedule call for new Customer account';
		    
		        caTk.OwnerId = rec.OwnerId;  
		        tks.add(caTk);
		    } else  {
		        Task dfTk = new Task();
		        dfTk.WhatId = rec.Id;
		        dfTk.Subject = 'Schedule call for new standard account';
		        dfTk.OwnerId = rec.OwnerId;
		        
		        tks.add(dfTk);
		    }
		}
		if(rec.is_supplier__c == true && rec.verification_status__c == 'Completed' 
		&& rec.AnnualRevenue > 1000000)
		{
		    String accountJson = JSON.serialize(rec);
		    HttpRequest req = new HttpRequest();
		    req.setEndpoint('api.netsuite/partners');
		    req.setMethod('POST');
		    req.setHeader('Content-Type', 'application/json');
		    req.setBody(accountJson);
		    Http http = new Http(); 
		    HttpResponse res = http.send(req);
		
		    if (res.getStatusCode() == 200)  {
		        System.debug('POST request successful. Response: ' + res.getBody());
		    } else  {
		        System.debug('POST request failed. Status code: ' + res.getStatusCode() + ', Response: ' + res.getBody());
		    }
		}
		
		insert tks;
}

This isn’t the worst code in the world. In fact, I'm sure you recognize the structure: a few if statements, a few for loops, and eventually a DML statement to wrap it all up. Every org I've worked on had methods that looked just like this one. And the code works; it fulfils the business requirement it was written for.

Now, how confident are you that you fully understand this code? Here are a few things that jump out at me when I read it:

  • The method name createTasks doesn’t tell us much. Is this a general-purpose method for creating Tasks? Why does it take an Account list as a parameter? Is this only meant to be used to create Tasks for Accounts? Can this be used to create Tasks in any situation? We don’t really know. We are forced to read the entire method in order to understand where and when it can be used.
  • The name of the Account list acctRecs isn’t clear. We can guess it stands for “account records,” but that’s just a guess. It could mean anything. Is it some naming convention we’re not aware of?
  • After the first if statement, we create a Task with the variable name paTk. What does paTk stand for? Is it “partner account task”? Maybe, maybe not. This pattern repeats in the other Task creation blocks. We don’t know what these names imply.
  • Notice we’re repeating the logic for creating a Task three times. The only difference is the content of the subject and category__c fields. If we need to change the logic for creating a Task, we have to change it in three places. That’s three spots where bugs can hide.
  • Before the HTTP callout, we repeat the first if statement we saw earlier in the method. We still don’t know what this condition represents, but assuming it represents the same concept in two different places, we again have two spots where bugs can hide if we later decide to change this condition.
  • What is this HTTP callout for? What’s its purpose? There’s nothing in the code to help us make an educated guess.
  • The formatting seems inconsistent. Occasionally, we have spaces before a new block of code starts, while other times we don’t. The intention isn’t consistent either.
  • Finally, we try to insert the tasks without handling any exceptions that might occur.

Now, let's go through a scenario where I refactor this code to make it cleaner. Refactoring involves changing the internal structure of the code without affecting its external behavior—in other words, the code will function the same but be much easier for other developers to read. We will talk about refactoring in more detail in future chapters.

We'll do this step-by-step, without fixing everything all at once. If you notice I skip something you'd like to see fixed, hang tight! I'll likely address it by the end of the chapter. Finally, don’t pay too much attention to the logic and whether it makes sense from a business point of view; this is merely an example to illustrate the concepts we will cover in the book.

Fixing the formatting

The first step I'm tackling is fixing the formatting. I've chosen to start here because it's the simplest way to enhance the cleanliness of this code. Additionally, I won't alter any of the logic just yet, ensuring that I don't inadvertently introduce any errors. Here's a breakdown of the changes I'll implement:

  • I'll insert a space between blocks of code. Whenever I open an if or for loop, there will be a space before the next block of code.
  • I'll ensure there's no space between closing parentheses and opening brackets.
  • I'll group related concepts closer together.
  • I'll incorporate general spacing throughout to provide breathing room for the mind.

Here's the updated version:

public static void createTasks(List<Account> acctRecs){

		List<Task> tks = new List<Task>();
		
		for(Account rec : acctRecs){
		
		    if(rec.is_supplier__c != false && rec.verification_status__c == 'Completed' && rec.AnnualRevenue > 1000000){
		    
		        Task paTk = new Task();
		        paTk.WhatId = rec.Id;
		        paTk.Subject = 'Schedule call for new Partner account';
		        paTk.category__c = 'Partner';
		        ppaTkat.OwnerId = rec.OwnerId;
		        
		        tks.add(paTk);
		    }
		    else if(rec.is_supplier__c != true && rec.AnnualRevenue < 1000000){
		    
		        Task caTk = new Task();
		        caTk.category__c = 'Customer';
		        caTk.WhatId = rec.Id;
		        caTk.Subject = 'Schedule call for new Customer account';
		        caTk.OwnerId = rec.OwnerId; 
		         
		        tks.add(caTk);
		    }
		    else{
		    
		        Task dfTk = new Task();
		        dfTk.WhatId = rec.Id;
		        dfTk.Subject = 'Schedule call for new standard account';
		        dfTk.OwnerId = rec.OwnerId;
		        
		        tks.add(dfTk);
		    }
		}
		
		if(rec.is_supplier__c == true && rec.verification_status__c == 'Completed' 
		&& rec.AnnualRevenue > 1000000){
		
		    String accountJson = JSON.serialize(rec);
		    
		    HttpRequest req = new HttpRequest();
		    req.setEndpoint('api.netsuite/partners');
		    req.setMethod('POST');
		    req.setHeader('Content-Type', 'application/json');
		    req.setBody(accountJson);
		    
		    Http http = new Http(); 
		    HttpResponse res = http.send(req);
		
		    if (res.getStatusCode() == 200){
		        System.debug('POST request successful. Response: ' + res.getBody());
		    } else  {
		        System.debug('POST request failed. Status code: ' + res.getStatusCode() + ', Response: ' + res.getBody());
		    }
		}
		
		insert tks;
}

Take a moment to flip through the pages and compare this version to the original one. You'll likely notice that even if we don’t fully understand what the code is doing or why, it's easier to read now. That's because the spacing provides our mind with a break, allowing us to absorb each concept before moving on to the next. This is a practice that's often overlooked, yet it's simple to implement and offers substantial rewards. Let's continue.

Fixing the method signature

Let's start by making the method name clearer. The name createTasks doesn’t tell us much about what type of Tasks we are creating and why. After reading the code, I figured out that it’s about creating Tasks for new Accounts, so I renamed the method to createTasksForNewAccounts. Additionally, it wasn’t clear where the parameter acctRecs was coming from. After looking around, I found that this method is called from an Account trigger, and the accounts passed to it are from the trigger context variable Trigger.New. With this new information, I’m going to rename the parameter to newAccounts.

Let’s see what the code looks like now with these two small changes:

Before

public static void createTasks(List<Account> acctRecs){
  ...
}

After


public static void createTasksForNewAccounts(List<Account> newAccounts){
  ...
}

That’s a lot easier to read. Just by looking at the method signature, we can immediately tell that this method is responsible for creating Tasks for new Accounts. Notice how much information we provided to the reader simply by using better names. Let's keep going.

Using better names

Let’s now focus on the variables immediately after the method signature. The list of tasks is called tks, which isn’t very intuitive; it’s also hard to pronounce and remember. I’m going to rename this to tasks. Then, the for loop variable is called rec, which I assume stands for “record”, but that’s not entirely clear. Since we are iterating over the new accounts, I’m going to call this account. Let’s see what the code looks like now.

Before

public static void createTasks(List<Account> acctRecs){

        List<Task> tks = new List<Task>();

        for(Account rec : acctRecs){

After

public static void createTasksForNewAccounts(List<Account> newAccounts){

        List<Task> tasks = new List<Task>();

        for(Account account : newAccounts){

Again, this is easier to read. We know that to create a Task, we first have to iterate over the new Accounts. Looking further down at the code, I’m concerned about the names we are using for the Task variables.

if(account.is_supplier__c != false && account.verification_status__c == 'Completed' && account.AnnualRevenue > 1000000){
		
		Task paTk = new Task();
		paTk.WhatId = account.Id;
		paTk.Subject = 'Schedule call for new Partner account';
		paTk.category__c = 'Partner';
		ppaTkat.OwnerId = account.OwnerId;
		
		tasks.add(paTk);
}
else if(account.is_supplier__c != true && account.AnnualRevenue < 1000000){

		Task caTk = new Task();
		caTk.category__c = 'Customer';
		caTk.WhatId = account.Id;
		caTk.Subject = 'Schedule call for new Customer account';
		caTk.OwnerId = account.OwnerId;  
		
		tasks.add(caTk);
}
else{

		Task dfTk = new Task();
		dfTk.WhatId = account.Id;
		dfTk.Subject = 'Schedule call for new standard account';
		dfTk.OwnerId = account.OwnerId;
		
		tasks.add(dfTk);
}

We see paTk, caTk, and dfTk. The issue with these names is that not only are they uncommon, but I also don’t know if they have a specific meaning I should be aware of. Maybe paTk is an internal acronym for a special type of Task record, or perhaps it’s a term well-recognized by the business. If I replace paTk with partnerTask, will I change the meaning of this logic?

Or perhaps it's just what the developer thought of in the moment as a short version of “partner account task”. How do I know which is correct? These uncertainties discourage me and other developers from modifying this code. Code that discourages other developers from changing it is not clean code.

Alright, assuming I did some research and concluded that these aren’t internal business names and I can safely update the names to something better, let's go ahead and make those changes:

if(account.is_supplier__c != false && account.verification_status__c == 'Completed' && account.AnnualRevenue > 1000000){
		
		Task partnerAccountTask = new Task();
		partnerAccountTask.WhatId = account.Id;
		partnerAccountTask.Subject = 'Schedule call for new Partner account';
		partnerAccountTask.category__c = 'Partner';
		partnerAccountTask.OwnerId = account.OwnerId;
		
		tasks.add(partnerAccountTask);
}
else if(account.is_supplier__c != true && account.AnnualRevenue < 1000000){

		Task customerAccountTask = new Task();
		customerAccountTask.category__c = 'Customer';
		customerAccountTask.WhatId = account.Id;
		customerAccountTask.Subject = 'Schedule call for new Customer account';
		customerAccountTask.OwnerId = account.OwnerId;  
		
		tasks.add(customerAccountTask);
}
else{

		Task defaultTask = new Task();
		defaultTask.WhatId = account.Id;
		defaultTask.Subject = 'Schedule call for new standard account';
		defaultTask.OwnerId = account.OwnerId;
		
		tasks.add(defaultTask);
}

This reads a lot better. Firstly, we're extremely clear with our intent and how one type of task differs from others. Secondly, we’ve removed doubt and uncertainty from the codebase. Some might argue that the previous names were shorter and thus easier on the eye, and while that may be true, the benefits of readability, certainty, and confidence far outweigh the temporary benefit of something that's easy on the eye.

Let’s see what else we can fix in this code.

Clarifying the boolean logic

I'll include the code here again for reference. This is what's contained within the for loop.

if(account.is_supplier__c != false && account.verification_status__c == 'Completed' && account.AnnualRevenue > 1000000){
		...
}
else if(account.is_supplier__c != true && account.AnnualRevenue < 1000000){

		...
}
else{

		...
}

There are several concerns I have about this code. First, the use of double negatives makes it hard to read. For example:

account.is_supplier__c != false

I find myself reading this out loud to make sense of it: “If the supplier field is not false.” Wouldn’t it be easier to just say, “If the supplier field is true?” Similarly, in the second if statement, we say, “if the supplier field is not true,” which would be clearer as “if the supplier field is false.” This kind of double negative logic can quickly become difficult to follow, especially when combined with additional boolean logic. For example, “if the supplier field is not false and the verification status is completed and the annual revenue is more than 1000000.” Let's make this logic a bit easier to understand.

if(account.is_supplier__c == true && account.verification_status__c == 'Completed' && account.AnnualRevenue > 1000000){
		...
}
else if(account.is_supplier__c == false && account.AnnualRevenue < 1000000){

		...
}
else{

		...
}

That’s better, but it’s not quite there yet. The following expression is redundant

account.is_supplier__c == true

Because the is_supplier__c field is a checkbox, it already has either a true or false value. So we can simply use the field itself as the evaluation criteria, like this

if(account.is_supplier__c && account.verification_status__c == 'Completed' && account.AnnualRevenue > 1000000){
		...
}
else if(!account.is_supplier__c && account.AnnualRevenue < 1000000){

		...
}
else{

		...
}

Notice that in the second if statement I’m using the ! operator to negate the boolean, which is the same as say “if the supplier field is false”. This is a lot more succinct. There’s less code to read and it gets straight to the point.

The second concern I have is why we're mixing the Account supplier field with the verification status and the annual revenue field. What do these three fields have in common? Why are they used together inside the if statement? Remember that if conditions are used to determine if a condition is true before we proceed. What is the condition we are evaluating here? I see the fields being evaluated, but I don’t understand the business context. I don’t understand how these three fields together represent a higher concept that needs to be true before we can proceed.

After some digging, I learned that the annual revenue determines the tier of the account, and that we only need to create Tasks for certain tiers. Additionally, Accounts that are tagged as suppliers are considered partner accounts, but only if the verification status is completed. Could you have guessed all this context from reading this code? I wouldn’t have. Most developers would address this by adding a comment with the ticket number of the requirement that led to the creation of this code, like this:

//https://myorg.atlassian.net/browse/FC-1220
//create tasks for accounts based on tier
if(account.is_supplier__c && account.verification_status__c == 'Completed' && account.AnnualRevenue > 1000000){

While this isn't so bad, wouldn’t it be better if the code itself was the documentation? Why rely on a third-party system of record for that? Let’s try that; I’m going to start by extracting the boolean logic to their own variables, like this:

Boolean isPartnerAccount = (account.is_supplier__c && account.verification_status__c == 'Completed');
Boolean isCustomerAccount = !account.is_supplier__c;
Boolean isGoldTier = account.AnnualRevenue > 1000000;
Boolean isSilverTier = account.AnnualRevenue < 1000000;

By moving the boolean expressions out of the if statements and into specific variables, I’m able to give them a name. Once I have the boolean logic extracted to variables, I can reuse these variables in the if statement to construct something like this:

if(isPartnerAccount && isGoldTier){
  ...
} 
else if(isCustomerAccount && isSilverTier){
  ...
} 
else {
  ...
}

Again, this works because both isPartnerAccount and isGoldTier are booleans, so I can combine them to create another boolean expression. Doesn’t this read like pure plain English? I can even read it out loud: “If this is a partner account and it’s a gold tier account, do this…”

Let’s see how else we can clean this code.

Reusability

Now, if the distinction between partner and customer Accounts is so important, surely we have logic all over our codebase that needs to differentiate between a partner and a customer account. The same is true for the boolean logic that determines the account tier. It's great that we extracted this logic into boolean variables, but those variables can't be reused by other classes. Let’s fix this by moving this logic to a domain class called AccountDomain:

public class AccountDomain {

public static Boolean isPartnerAccount(Account account){
	...
}

public static Boolean isGoldTier(Account account){
  ...
}

Now, this logic can be reused by any other class. Also, because we created methods instead of simple variables, we can add as much logic as we want inside of them. For example, if the requirements change in the future (and they will) on how we determine that an account is a Gold Tier one, we can simply modify this method, and automatically all other code that depends on it will start using the new logic.

Let’s see what the code looks like now

Before

if(rec.Name.containsIgnoreCase('partner') && rec.AnnualRevenue > 1000000){
  ...
} else if(rec.Name.containsIgnoreCase('customer') && rec.AnnualRevenue > 5000){
  ...
} else {
   ...
}

After

Boolean isPartnerAccount = AccountDomain.isPartnerAccount(account);
Boolean isCustomerAccount = AccountDomain.isCustomerAccountAccount(account);
Boolean isGoldTier = AccountDomain.isGoldTier(account);
Boolean isSilverTier = AccountDomain.isSilverTier(account);

if(isPartnerAccount && isGoldTier){
  ...
} 
else if(isCustomerAccount && isSilverTier){
  ...
} 
else {
  ...
}

Your initial reaction may be “but we ended up writing a lot more code!” And you're right, I get it. Clean code can often result in more code than the quick-and-dirty code. But let’s review the benefits so far:

  • The boolean logic now reads like plain English.
  • We’ve made the code self-documenting. We understand the relationship between account tiers and account types without having to reference old ticket numbers.
  • We’ve made some of this logic reusable in other parts of our codebase, and in doing so,
  • We’ve introduced a new concept into our codebase; the concept of tiers and account types. This makes our codebase more language-rich.

But we're not done yet.

Don’t repeat yourself too much

There are a few more opportunities for reusing some of this logic. If we look at the original code (but with the new boolean expressions), we’ll see that the code for creating tasks is almost the same for each condition.

if(isPartnerAccount && isGoldTier){

		Task partnerAccountTask = new Task();
		partnerAccountTask.WhatId = account.Id;
		partnerAccountTask.Subject = 'Schedule call for new Partner account';
		partnerAccountTask.category__c = 'Partner';
		partnerAccountTask.OwnerId = account.OwnerId;
		
		tasks.add(partnerAccountTask);
} 
else if(isCustomerAccount && isSilverTier){ 

		Task customerAccountTask = new Task();
		customerAccountTask.category__c = 'Customer';
		customerAccountTask.WhatId = account.Id;
		customerAccountTask.Subject = 'Schedule call for new Customer account';
		customerAccountTask.OwnerId = account.OwnerId;  
		
		tasks.add(customerAccountTask);
 }
 else {
 
		Task defaultTask = new Task();
		defaultTask.WhatId = account.Id;
		defaultTask.Subject = 'Schedule call for new standard account';
		defaultTask.OwnerId = account.OwnerId;
		
		tasks.add(defaultTask);
 }

The only thing that differs is how we populate the subject and category__c fields. However, notice that in the third condition, we aren’t populating the category__c field. Why not? Is this by design? Is this a bug? Was there a requirement that this field only needs to be populated for Accounts of a certain tier? Such a simple omission can lead to many questions, and not having clear answers to these questions makes the code hard to reason about and hard to change. I did some research and found out that no one knows why this field is even there and why it’s not populated for this condition. I don’t have a solution to this problem, so I’ll have to leave this logic as is. I included this example to show that clean code is also about not creating confusion for the reader (which could be your future self long after you’ve forgotten why you wrote this code).

Moving on, the next problem is that aside from these two fields, the logic for creating a task is the same. If we need to change this logic, we have to update it in three different places. What if we forget to update one of the blocks? That’s probably what happened when the previous developer added the category__c field. We can fix this by simply extracting this logic into a private method in this class. Like this:

if(isPartnerAccount && isGoldTier){

		Task partnerAccountTask = getBaseTask(account);
		partnerAccountTask.category__c = 'Partner';
		partnerAccountTask.Subject = 'Schedule call for new Partner account';
		
		tasks.add(partnerAccountTask);
} 
else if(isCustomerAccount && isSilverTier){

		Task customerAccountTask = getBaseTask(account);
		customerAccountTask.category__c = 'Customer';
		customerAccountTask.Subject = 'Schedule call for new Customer account';
		
		tasks.add(customerAccountTask);
} 
else{

		Task defaultTask = getBaseTask(account);
		defaultTask.Subject = 'Schedule call for new standard account';
		
		tasks.add(partnerAccountTask);
}

This looks better. The getBaseTask() method generates a task with default fields, but we can still incorporate specific logic inside each condition. This way, we can reuse some logic while still maintaining some control over it.

We can clean this up even further by wrapping those blocks of code into their own methods, like this

private static Task createPartnerAcccountTask(Account account){

	Task task = getBaseTask(account);
	task.category__c = 'Partner';
	task.Subject = 'Schedule call for new Partner account';
	
	return task;
}

Notice that inside this new method, we are still reusing the generic logic for creating tasks that we defined inside the getBaseTask() method. Let’s assume I did the same for the other blocks of code. With this change, the code would now look like this:

if(isPartnerAccount && isGoldTier){
		
		tasks.add(createPartnerAccountTask(account));
} 
else if(isCustomerAccount && isSilverTier){

		tasks.add(createCustomerAccountTask(account));
} 
else{
		
		tasks.add(tasks.add(createDefaultTask(account)););
}

At this point, I worry that I may have gone too far. Did I really need to create the getBaseTask() method and then a specific method for each type of Task? On one hand, the code is much easier to read. On the other hand, understanding the end-to-end flow now requires drilling down into several small methods. Knowing when to abstract a piece of logic into its own method is a balancing act, often more art than science, combined with experience.

In this particular case, having one method per Task type would be beneficial if we anticipate changes in how each task is created. For example, if a new requirement mandates modifying how partner tasks are created, I would only need to update the createPartnerAccountTask() method without worrying about anything else. Without such a method, I’d have to comb through the code to find where partner tasks are created.

As I said, this is a balancing act, and we will explore the pros and cons of this approach in future chapters.

Putting it all together.

I'm going to skip some of the other improvements to the code because they are simple variations of the principles we just went over. After refactoring the entire method, here's what I ended up with:

public static void createTasksForNewAccounts(List<Account> newAccounts){

		List<Task> tasks = new List<Task>();
		
		for(Account account : newAccounts){
		  
		    Boolean isPartnerAccount = AccountDomain.isPartnerAccount(account);
				Boolean isCustomerAccount = AccountDomain.isCustomerAccountAccount(account);
				Boolean isGoldTier = AccountDomain.isGoldTier(account);
				Boolean isSilverTier = AccountDomain.isSilverTier(account);
				
				if(isPartnerAccount && isGoldTier){
		
						tasks.add(createPartnerAccountTask(account));
				} 
				else if(isCustomerAccount && isSilverTier){
				
						tasks.add(createCustomerAccountTask(account));
				} 
				else{
						
						tasks.add(tasks.add(createDefaultTask(account)););
				}
		
		    if(isPartnerAccount){
				    syncAccountWithNetsuite(account);
		    }
		}
		
		try {
		    insert tasks;
		} catch (Exception e) {
		    Logger.logError(tasks,e);
		}
}

Read this from top to bottom a few times. You’ll see it reads in plain English, almost as if someone were verbally explaining what the code does. In fact, let’s try that. I can read this out loud as:

  • To create new tasks for accounts, we loop over the new accounts.
  • We determine whether the accounts are partner or customer accounts, and their tier.
  • If the account is a partner and gold tier, we create a partner account task.
  • If the account is a customer and silver tier...

Do you know what this reads like? It reads just like the acceptance criteria of a user story. Wouldn’t it be amazing if all of our code read exactly like the requirement it was written for? This is what clean code looks like.

Except…look at the syncAccountWithNetsuite() method. Why is this code here? How is creating tasks related to syncing partner accounts with NetSuite? Well, it turns out, it actually has nothing to do with it. What probably happened is that the developer received two requirements at the same time, both triggered by checking if the account is a partner account. So, the code was added here since they were already checking the account type. What’s worse is that now, every time we call the createTasksForNewAccounts() method, we make multiple API calls to NetSuite.

This is known as a side effect, and this one is particularly nasty. The main code claims to do one thing (create tasks) but also does something completely different, which can even modify data in a third-party system. We will explore this anti-pattern in more detail in future chapters.

In this case, the correct solution would be to extract this completely out of this method, and add it somewhere else in the trigger handler class, while also reusing our domain class, like this:

//somewhere in the trigger handler class

if(AccountDomain.isPartnerAccount(account)){
		syncAccountWithNetsuite(account);
}

An even better design would be to encapsulate the logic that deals with the NetSuite API into its own class, like this

//somewhere in the trigger handler class
if(AccountDomain.isPartnerAccount(account)){
		NetSuiteAPI.syncPartnerAccount(account);
}

I saved this for last purpose to make the point that clean code isn't just about readable names; it’s also about the design of our software: how loosely coupled it is, whether it's modular, and whether it has cohesion. These are concepts we will explore in future chapters.

Conclusion

I hope you enjoyed this exercise. We covered several principles of clean code at a high level, but there’s still much more to explore. We also witnessed the process of gradually refining legacy code without making drastic changes all at once. Let’s wrap up by revisiting our definition of clean code once more:

  • Clean code is easy and enjoyable to read.
  • It adheres to common and well-understood design patterns and best practices.
  • It clearly communicates its intention.
  • It's easy to modify safely and efficiently.
  • It's simple to test.

The updated version of the code was notably straightforward to read. The purpose of every variable and method was evident, leaving no room for ambiguity or uncertainty. When modifications are needed, it’s straightforward to pinpoint where those changes should occur.

There’s still more we could’ve done to make this code cleaner and modular; this was just a brief introduction. In the upcoming chapters, we'll delve deeper into the principles behind some of the decisions I made while refactoring this code. Specifically, in the next chapter, we'll explore one of the most crucial aspects of clean code: choosing good names.

Subscribe for exclusive Salesforce Engineering tips, expert DevOps content, and previews from my book 'Clean Apex Code' – by the creator of HappySoup.io!
fullstackdev@pro.com
Subscribe