Clean Code on the Force.com Platform

A month ago, on Thursday July 10, 2014, I spoke at the Lehigh Valley Salesforce Developer User Group (@LVSFDCDUG) on “Clean Code and Software Professionalism on the Force.com Platform”. In this article, I would like to focus on the second half of my presentation on Clean Code. If you are interested, please review the slides I have posted to the meetup. Also, if you haven’t had a chance, read up on my article on Software Professionalism covering the first half of the presentation.


One of the most important aspects of being a software professional is writing clean code. Before I go into examples, I want to take a second to call out the fact that some of the things I am going to talk about here are subjective. You may have a different opinion that I do, and that is perfectly fine. The end goal is to write the best code possible, so please, by all means, write code in a way that works for you.


Wid__c w = new Wid__c();

public void process(Id par){
	if(par == null){
		throw new Exception();
	}
	
	w.Par__c = par;
	insert w;
}

Any guesses what the above code does? We see there is a w and that we want to process it. Ah, par appears to be important. We set the par and then we insert it. Seems straight forward enough right? Well, let’s see if we can make it a bit easier to understand.


Widget__c widget = new Widget__c();

public void addWidgetToAccount(Id parentAccountId){
	if(parentAccountId == null){
		throw new MissingParentAccountIdException();
	}
	
	widget.ParentAccount__c = parentAccountId;
	
	Savepoint sp = Database.setSavepoint();
	try{
		insert widget;
	}catch(Exception e){
		Database.rollback(sp);
	}
}

This paints a much clearer picture. We now know that w was actually a widget. By being more verbose with the exception, we know that an error is thrown when the parentAccountId was missing. It is important to note that by being more specific and using a specific exception, we can provide more precise error handling as well. The final important thing to note is the additional of error handling around the insert logic. While the original code would work perfectly fine, this added bit of error handling is a good sign of taking your code to the next level. You should be focusing on never letting anything negative get to the end user unless you present it in the proper way. Remember, compiling is never enough. You code should be better than just compiling. Proper error handling with readability is important.

With all of that said, I argue we can make it even better.


WWidget__c widget = new Widget__c();

public void addWidgetToAccount(Id parentAccountId){
	verifyParentAccountOnWidget(parentAccountId);
	setParentAccountOnWidget(parentAccountId);
	insertWidget();
}

private void verifyParentAccountIdSet(Id parentAccountId){
	if(parentAccountId == null){
		throw new MissingParentAccountIdException();
	}
}

private void setParentAccountOnWidget(Id parentAccountId){
	widget.ParentAccount__c = parentAccountId;
}

private void insertWidget(){
	Savepoint sp = Database.setSavepoint();
	try{
		insert widget;
	}catch(Exception e){
		Database.rollback(sp);
	}
}

This specific example may be a bit overkill, but for the sake of simplicity, I wanted to leave it. Notice how we broke out the logic into different methods. This provides better reusability, but it also makes understanding what this code does that much easier. We can easily figure out addWidgetToAccount does. First, it will verifyParentAccountOnWidget, then setParentAccountOnWidget, finishing it up with insertWidget. Anyone can read that. There is an added benefit of keeping your methods small that makes debugging simpler.

References


public void insertTenWidgets(){
	for(Integer i = 0; i < 10; i++){
		Widget__c widget = new Widget__c();
		insert widget;
	}
}

This is the perfect example of code that compiles, but just isn’t good enough. This is the type of issue that will plague you in the long run too, because you won’t notice it right away. The issue here of course is that we have a DML statement inside of a for loop. How would we change it?


public void insertTenWidgets(){
	List<Widget__c> widgets = new List<Widget__c>();
	for(Integer i = 0; i < 10; i++){
		Widget__c widget = new Widget__c();
		widgets.add(widget);
	}
	insert widgets;
}

When we refactor it to properly only call a single DML statement on the List of widgets, we conserve DML statements and ensure that our code is bulkified.


public List<Widget__c> getWidgetsByType(List<String> types){
	List<Widget__c> allWidgetsByType = new List<Widget__c>();
	for(String type:types){
		List<Widget__c> widgetByType = [
			SELECT
				Name
			FROM
				Widget__c
			WHERE
				Type = :type
		];
		allWidgetsByType.addAll(widgetByType);
	}
	return allWidgetsByType;
}

In a similar situation as above, we have some inefficient code that will compile and work perfectly fine under most circumstances. However, we can make it much better.


public List<Widget__c> getWidgetsByType(List<String> types){
	List<Widget__c> allWidgetsByType = [
		SELECT
			Name
		FROM
			Widget__c
		WHERE
			Type IN :types
	];
	return allWidgetsByType;
}

Not only are we able to reduce SOQL statements, but we are also able to reduce Apex statements. By embracing clean code, we take on the responsibility to know as much about a language as possible to ensure we write our code to perform as well as it can. While this may seem like a minor tweak, tweaking all of your code to be just slightly more efficient can have a tremendous impact, especially considering dealing with the data layer is usually the most time consuming step of the process.


trigger WidgetBeforeInsertTrigger on Widget__c (before insert){
	Widget__c widget = Trigger.new[0];
	List<Account> accounts = [
		SELECT
			Id, Name, Widget_Count__c
		FROM
			Account
		WHERE
			Id = :widget.ParentAccount__c
	];
	// do something with accounts
}

There are two issues here. The first is obvious. We should never write a Trigger to only handle a single object. It should always be written to handle bulk scenarios. The other issue is subjective, but the issue is that this Trigger is named WidgetBeforeInsertTrigger and uses no context variables which indicates that this Trigger was written to only handle before insert scenarios. This would lead to multiple Triggers per object which is typically considered a bad practice.


trigger WidgetTrigger on Widget__c (before insert, before update){
	if(Trigger.isBefore && Trigger.isInsert){
		WidgetTriggerHandler.handleAccountManipulation(Trigger.new);
	}
}

By utilizing context variables, we are now able to write a single Trigger for every combination. This makes your Triggers easier to maintain. On top of that, we took the logic we were including directly in our Trigger and moved it out into a separate class. This allows us to keep our Trigger cleaner (especially when you need to handle more than one scenario) and provides greater reusability.

References


public class WidgetController{
	private Widget__c widget;
	private List<Contact> contacts;
	
	public WidgetController(ApexPages.StandardController stdController){
		widget = (Widget__c)stdController.getRecord();
		contacts = [
			SELECT
				Id, Name
			FROM
				Contact
			WHERE
				Widget_Type__c = :widget.Type__c
		];
	}
	
	public PageReference save(){
		performWidgetCalculation(widget);
		saveWidget(widget);
		PageReference widgetPage = ApexPages.StandardController(widget).view();
		widgetPage.setRedirect(true);
		return widgetPage;
	}
	
	private void performWidgetCalculation(Widget__c widget){
		// Any complex logic
	}
	
	private void saveWidget(Widget__c widget){
		// Save logic with error handling
	}
}

Now we are getting into the more advanced topics (which subsequently are more subjective and could be argued several ways). The problem with the above piece of code is that it has poor Separation of Concerns. The general idea here is that your Controller shouldn’t be responsible for handling all of the logic on your page. This is true for several reasons, but generally it makes your code unmanagable over time and provides duplicated efforts. The odds are that some of the code in your controller can be reused elsewhere. For instance, getting Contacts by Widget__r.Type__c can most likely be reused elsewhere. Not only would this code need to be written twice, it will need to be tested twice, and over time may become a nightmare if for instance you now need to add an additional field to your query.


public class WidgetController{
	private Widget__c widget;
	private List<Contact> contacts;
	
	public WidgetController(ApexPages.StandardController stdController){
		widget = (Widget__c)stdController.getRecord();
		contacts = WidgetService.getContactsByType();
	}
	
	public PageReference save(){
		WidgetService.performWidgetCalculation(widget);
		WidgetService.saveWidget(widget);
		PageReference widgetPage = ApexPages.StandardController(widget).view();
		widgetPage.setRedirect(true);
		return widgetPage;
	}
}

You can see in the above code we removed all of the logic needed for querying data, storing data, and performing complex calculations. Our controller can completely focus on doing exactly what it should be, controlling the view. I won’t go into too many details around all of this, but I highly suggest taking some time and reviewing some of the following references for the Apex Enterprise Patterns. Andy Fawcett (@andyinthecloud) did a fantastic job laying it all out, much better than I could. Take some time and read it if you have not already.

Apex Enterprise Patterns


@isTest
public class WidgetControllerTest{
	static testMethod void testConstructor(){
		Account account = new Account(
			Name = 'Test'
		);
		insert account;
		Widget__c widget = new Widget__c(
			ParentAccount__c = account.Id
		);
		insert widget;
		
		// Test logic
	}
	
	static testMethod void testSave(){
		Account account = new Account(
			Name = 'Test'
		);
		insert account;
		Widget__c widget = new Widget__c(
			ParentAccount__c = account.Id
		);
		insert widget;
		
		// Test logic
	}
}

How many times have you run across unit tests that resemble this? Now in this specific scenario, it isn’t too bad to generate these two objects like this. However, consider how this would look if you needed 50 objects for your test and you needed 1000 tests. This same piece of code may need to be written 50,000 times. The second you go to deploy it, you find out the Admin changed your Widget__c and now requires an additional field. Good luck updating all 50,000 references.


@isTest
public class WidgetControllerTest{
	static testMethod void testConstructor(){
		List<Widget__c> widgets = generateTestData();
		
		// Test logic
	}
	
	static testMethod void testSave(){
		List<Widget__c> widgets = generateTestData();
		
		// Test logic
	}
	
	private static List<Widget__c> generateTestData(){
		List<Account> account = TestUtils.generateAccounts(1);
		insert accounts;
		List<Widget__c> widgets = TestUtils.generateWidgetsWithAccounts(accounts);
		insert widgets;
		
		return widgets;
	}
}

Instead, let’s take that logic and encapsulate it to make it easier to reuse. Notice that we now have a TestUtils class for generating objects. We also made a simple method for generating the entire data model so we don’t need to include it in each test. This is a great change, but let’s take it another step further.


@isTest
public class WidgetControllerTest{
	static testMethod void testConstructor(){
		Map<String, List<SObject>> objectsByName = TestUtils.generateTestData();
		List<Widget__c> widgets = objectsByName.get('Widget__c');
		
		// Test logic
	}
	
	static testMethod void testSave(){
		Map<String, List<SObject>> objectsByName = TestUtils.generateTestData();
		List<Widget__c> widgets = objectsByName.get('Widget__c');
	
		// Test logic
	}
}

First, let’s focus on the changes we made to the above test class. Notice how our TestUtils is now generating the entire data model and returning it as a Map>. This is done so that we can easily retrieve all of the objects from the generated data. This is superior to the way we were generating the data model before due to the fact that we can grab any other objects rather than just the Widget__c

@isTest
public class TestUtils{
	public Map<String, List<SObject>> generateTestData(){
		Map<String, List<SObject>> testDataByName = new Map<String, List<SObject>>();
		List<Account> accounts = generateAccounts(1);
		testDataByName.add('Account', accounts);
		List<Widget__c> widgets = generateWidgetsFromAccounts(accounts);
		testDataByName.add('Widget__c', widgets);
		
		return testDataByName;
	}
	
	public List<Account> generateAccounts(Integer countOfAccounts){
		// do logic here
	}
	
	public List<Widget__c> generateWidgetsFromAccounts(List<Account> accounts){
		// do logic here
	}
}

The TestUtils class is used to generate the data model. I won’t go into too many details here, but I highly suggest reading my Proper Unit Test Structure in Apex article which dives into more detail.


@isTest
public class WidgetServiceTest{
	static testMethod void testService(){
		List<Widget__c> widgets = TestUtils.generateWidgets();
		
		WidgetService.getWidgets();
		WidgetService.calculateWidgets(widgets);
		WidgetService.saveWidgets(widgets);
		// every other method in the service
	}
}

Have you ever seen this? One of the most egregious pieces of code that can be written. This is a way to bypass actually writing unit tests. It can pass Salesforce’s minimum requirement for 75% code coverage, but honestly this is one of the most detrimental things you can do to a piece of code. It is absolutely crucial to provide real unit test coverage otherwise refactoring code becomes practically impossible. On top of that, the value of being able to click a button and with relative certainty say that the changes you made work (and don’t negatively effect the rest of the system) is too good to miss. Don’t ever fall for this trap. It may feel like the quick way to get through a jam, but it will come back to bite you and cost so much more time than just writing proper unit tests up front.


@isTest
public class WidgetServiceTest{
	static testMethod void testGetWidgets(){
		List<Widget__c> widgets = TestUtils.generateWidgets();
		
		Test.startTest();
		List<Widget__c> widgetsFromService = WidgetService.getWidgets();
		Test.stopTest();
		
		System.assertEquals(3, widgetsFromService.size(), '3 widgets match the criteria');
	}
	
	static testMethod void testCalculateWidgets(){
		// Test for WidgetService.calculateWidgets();
	}
	
	static testMethod void testSaveWidgets(){
		// Test for WidgetService.saveWidgets();
	}
}

We now have unit tests for each of our methods. Those methods all now properly generate the data model and even more importantly assert something. This example of an assert statement is actually very weak, but it is just an example. You should assert specific information and assert as much as reasonably possible. This will guarantee that your code actually does what you expect it to do. As a developer, an assert statement is the equivalent of proving that the code we wrote actually works the way we expect it to work. It is our responsibility to ensure that is true.

References


The final, and maybe most crucial, aspect of writing clean code on the Force.com platform is that in most cases, you will be better off not writing any code at all. As developers, our first instinct is to solve every problem with some code. As someone who originally comes from a multitude of different languages, it was a bit of a jolt to realize that code wasn’t always the right solution. There are a few key facts to keep in mind though:

  • The features backed into the platform will always be faster than any Apex you can write.
  • Those features are tested much more extensively than any piece of Apex you can write.
  • It is almost always faster to use a feature of the platform than to write the Apex code for it.

Every great Salesforce developer also has some great administrator qualities. It is important to never take that for granted. Make sure you keep up with the features natively available to you. While doing so, it is also important to remember that production is sacred. One of the traps many people fall into is the fact that Salesforce provides a high level of flexibility and customization with very little technical knowledge required. However, as professionals, it is our responsibility to teach our fellow developers and administrators that metadata changes directly in production should never be done. Any metadata change can cause unforeseen issues. For instance, a developer may have built a custom page in Visualforce for entering contacts. If you make a change in production, maybe as simple as marking a new field required, that form may break if it doesn’t have that field already on it. Every change should go through some level of testing and promoted to production only after testing has been completed. Use any of the several deployment tools to make that possible (this also has the added benefit of running your unit tests again, making sure that you aren’t breaking any of them either).

References


I would like to give credit to the above book, Clean Code: A Handbook of Agile Software Craftsmanship by Robert Martin (@unclebobmartin) for heavily influencing my presentation and this article. Robert Martin, aka Uncle Bob, has been a huge positive influence on my career. This is an amazing book and I highly suggest it for anyone who is interested.


Important Note: It is important to remember that these thoughts are my personal opinion. As with any opinion, it may or may not reflect the opinion of any organization I am associated with.

8 Responses to “Clean Code on the Force.com Platform”

  1. August 19, 2014 at 6:32 am #

    Excellent article, Jesse. I would love to see a clean code movement in Force.com really take hold and be mainstream. The platform makes it very easy to create fairly useful functionality without much background in software engineering, which is not a bad thing in and of itself. It may be OK for bits of “Apex glue”, but doesn’t necessarily scale well to larger enterprise systems/apps. An article like this can definitely help those looking to advance and do things the right way. Very nice.

  2. August 20, 2014 at 7:19 pm #

    Excellent post. We are a FF customer and an ISV partner of SFDC with our own native Force.com offering. There is certainly an art and discipline to developing robust, re-usable, well-behaved code-based solutions on any technology stack but the benefits of Salesforce.com’s multi-tenant env. pose additional or at least different constraints that professional Force.com developers must overcome. That said, I think your article advances the art and organizes many essential concepts and principles. Well done and thanks! This topic alone should be a core session at Dreamforce 2014.

    • August 21, 2014 at 1:47 pm #

      I would have loved to do a presentation at Dreamforce on this topic, but unfortunately my submission did not get selected. I will try again next year! Thanks for reading the article and for all the great feedback!

  3. August 29, 2014 at 1:31 pm #

    Excellent article. I’m always for clean, readable, and maintainable code – especially when multiple people are responsible for that code over time. Writing good code is a lost art. Back in the days when CPU cycles were critical, you wrote code that was efficient because otherwise your machine would crash. Comments were a must too. Nowadays, way too many developers write code that just runs, without any thought to how well it runs and what happens when someone tries to understand their logic in 6 months.

    • August 30, 2014 at 4:26 pm #

      I can’t agree with you enough. Compiling is just not good enough. For almost all code, most of the time spent on it will be during maintenance. That is why it is so important to make it easy to modify and maintain. Shedding a few minutes during the initial creation of the code will hurt much worse in the future.

      To maintain good coding practices, I like the following saying: “Always code as if the guy who ends up maintaining your code will be a violent psychopath who knows where you live.”

      Makes you think twice about that commit 😉

  4. August 31, 2014 at 8:31 pm #

    Great article Jesse, it really helps having the before and after examples.

    One small point if I may:

    “Notice how we broke out the logic into different methods. This provides better reusability, but it also makes understanding what this code does that much easier.”

    I assume by “reusability” here, you mean reusability within the class, as obviously the private methods wouldn’t be visible outside of the class. I tend to bundle such methods together in a Services (or similar) class, either because such methods are useful elsewhere (which the methods in the example could be), or simply as an organisation-of-code measure. It also helps avoid having useful methods in oddly named classes e.g. OpportunityHandler.getAccountsFromIDs()

    But like I said, I think you meant inside the class anyway!

    • September 1, 2014 at 9:35 am #

      Hey Gary. Thanks for the comment! In this particular instance, I did mean inside the class. I agree, typically when talking about reusability you want to refer to a Service that has public methods. However, reusability inside its own class can still provide benefits. In that particular instance, the reason I refactored it like that was more about readability than reusability, I just wanted to point it out as it is an added benefit.

Trackbacks/Pingbacks

  1. Software Professionalism on the Force.com Platform | Jesse Altman - August 18, 2014

    […] week, I’ll review the second half of my presentation on Clean Code. I’ll dive into the code examples and explain key factors to consider when developing on the […]

Leave a Comment