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.


[java]
Wid__c w = new Wid__c();

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

w.Par__c = par;
insert w;
}
[/java]

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.


[java]
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);
}
}
[/java]

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.


[java]
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);
}
}
[/java]

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


[java]
public void insertTenWidgets(){
for(Integer i = 0; i < 10; i++){ Widget__c widget = new Widget__c(); insert widget; } } [/java] 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?


[java]
public void insertTenWidgets(){
List widgets = new List();
for(Integer i = 0; i < 10; i++){ Widget__c widget = new Widget__c(); widgets.add(widget); } insert widgets; } [/java] 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.


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

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.


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

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.


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

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.


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

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


[java]
public class WidgetController{
private Widget__c widget;
private List 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
}
}
[/java]

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.


[java]
public class WidgetController{
private Widget__c widget;
private List 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;
}
}
[/java]

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


[java]
@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
}
}
[/java]

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.


[java]
@isTest
public class WidgetControllerTest{
static testMethod void testConstructor(){
List widgets = generateTestData();

// Test logic
}

static testMethod void testSave(){
List widgets = generateTestData();

// Test logic
}

private static List generateTestData(){
List account = TestUtils.generateAccounts(1);
insert accounts;
List widgets = TestUtils.generateWidgetsWithAccounts(accounts);
insert widgets;

return widgets;
}
}
[/java]

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.


[java]
@isTest
public class WidgetControllerTest{
static testMethod void testConstructor(){
Map> objectsByName = TestUtils.generateTestData();
List widgets = objectsByName.get(‘Widget__c’);

// Test logic
}

static testMethod void testSave(){
Map> objectsByName = TestUtils.generateTestData();
List widgets = objectsByName.get(‘Widget__c’);

// Test logic
}
}
[/java]

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

[java]
@isTest
public class TestUtils{
public Map> generateTestData(){
Map> testDataByName = new Map>();
List accounts = generateAccounts(1);
testDataByName.add(‘Account’, accounts);
List widgets = generateWidgetsFromAccounts(accounts);
testDataByName.add(‘Widget__c’, widgets);

return testDataByName;
}

public List generateAccounts(Integer countOfAccounts){
// do logic here
}

public List generateWidgetsFromAccounts(List accounts){
// do logic here
}
}
[/java]

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.


[java]
@isTest
public class WidgetServiceTest{
static testMethod void testService(){
List widgets = TestUtils.generateWidgets();

WidgetService.getWidgets();
WidgetService.calculateWidgets(widgets);
WidgetService.saveWidgets(widgets);
// every other method in the service
}
}
[/java]

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.


[java]
@isTest
public class WidgetServiceTest{
static testMethod void testGetWidgets(){
List widgets = TestUtils.generateWidgets();

Test.startTest();
List 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();
}
}
[/java]

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.

Advertisement

Go to Smartblog Theme Options -> Ad Management to enter your ad code (300x250)

Comments are closed.