At OpFocus, we often review code written by other developers. Sometimes these code reviews are planned: a client invites us in to review their code, either for training purposes or to address a specific problem. Other times, code reviews are a response to an unexpected emergency: for example, we try to deploy new functionality only to discover that existing unit tests aren’t passing in Production, or, we get a panicked call from a client saying that they’re suddenly seeing Governor Limit errors in code that a long-gone system administrator wrote, and we’re asked to fix the problem that is blocking users from saving their data.

Beginning a review of someone else’s code is always a little stressful. It’s like being called in to clean someone else’s house, where you have to get it clean in 2 hours, but you have no idea how many rooms need to be cleaned or how messy those rooms might be when you start the job. I imagine that house cleaners have pet peeves when they begin a cleaning job (probably things like ferocious pets or unwashed dishes in the kitchen sink.) I certainly have pet peeves when I conduct a code review; things that I find that raise my blood pressure because I know they’re not serving our clients well. (On the bright side, it is gratifying to know that OpFocus can clean these things up for our clients!) Here are a few of my own pet peeves for code reviews, and what you should keep in mind as Best Practices for writing code.

5 Pet Peeves for Salesforce.com Code Reviews

1. No Comments

Sometimes I’ll see a trigger with a hundred lines of code, with absolutely no comments except for one line, buried deep within the code that says:

    // Increment x

I’ve never understood why some developers don’t take time to comment their code! Every Trigger, Class, and method should begin with a comment block that explains what the code does. Every section of code within a Trigger or Class should contain an explanation as well. (For that matter, complex formulas in Custom Fields, Validation Rules, and Approval Processes should also have comments, but that’s a story for another time.)

It doesn’t take long at all to write a comment. Even a mediocrely-written comment can save a ton of time months later, when someone else is trying to make sense of your code. Please, as you’re writing your code, write comments!

2. Non-Bulkified Triggers

Every couple of months, we run across non-Bulkified (or poorly Bulkified) Triggers. These Triggers are usually written by people who are experienced in Java or some other language, but who aren’t used to developing in Apex, and who don’t understand the factors that Salesforce’s Governor Limits require developers to consider. All triggers must be bulkified! This may sound harsh, but if you don’t know what that means, think long and hard about whether you should be developing code that will impact your company’s Production Users! At a minimum, read the Apex Language Reference, especially the section on “Running Apex Within Governor Execution Limits.” And consider bringing in an experienced consultant to review your code and suggest improvements. A little experienced help as you’re learning can help save you a lot of time (and your company a lot of money) in the long term!

3. Hard-Coded Ids

Very often, I’ll run across code with hard-coded Ids like this:

    Contact c = new Contact();
    c.RecordTypeId = '012700000001du7';

Hard-coded Ids – augh! They’re not self-documenting – looking at the above code, can you tell why it’s assigning that particular Record Type Id to the new Contact, or what Record Type is being assigned? But even worse, if the Record Type was just created in the Sandbox, when the code is deployed and the new Record Type is created in Production, it’ll have a different Id, and the code above will fail.

A much better version of this code would be:

    RecordType rtInternalContact = [select id from RecordType 
      where SObjectType='Contact' 
      and Name='Internal Contact'];
    Contact c = new Contact();
    c.RecordTypeId = rtInternalContact.id;

Even though this code doesn’t have any comments, it’s much easier to see what Record Type is being used. And if the “Internal Contact” Record Type has a different Id in Production, this code will use that new Id without any changes. (Don’t forget to bulkify this code – you don’t want to make this query inside a FOR loop!)

(You can also get a Record Type Id using the DescribeSObjectResult object’s getRecordTypeInfos*() methods. See the Apex Language Reference for details.)

4. Unit Tests That Don’t Test

Sometimes, when we look at a Unit Test, we can tell exactly how and when it was written: in a panic, when the original developer tried to deploy some code and realized that they hadn’t written any Unit Tests for it. When that happens, the developer sometimes slaps together Unit Tests that might achieve the code coverage required for deployment, but that don’t actually test anything. How frustrating! The whole purpose of unit tests is to help ensure quality: slapping together unit tests that don’t verify anything only serves to help you deploy potentially poor-quality code to Production!

Unit Tests should test both positive and negative cases. Unit Tests for Triggers should test both individual and bulk DML operations. When you find a problem in your code, you should consider developing a regression Unit Test to prevent a recurrence of the same problem. Developers (and their management) often perceive Unit Tests as a necessary evil. I encourage you to re-evaluate that opinion. A well-written Unit Test can help you find and solve problems long before they’re experienced in Production. Unit tests are your friend!

At OpFocus, all of our development estimates include time to write Unit Tests. We don’t consider a coding assignment to be done until adequate Unit Tests have been written for it. We don’t wait until deployment time; we write tests as we go. I strongly encourage you to adopt the same practice.

5. Bogus “i = i + 1” Code for Unit Test Code Coverage

Every now and then, I’ll run into an org that is at 90% or more of its Apex code limit, and that has one or two very long Apex Classes that have methods with thousands of lines of code like this:

    Integer i;
    i = i + 1;
    i = i + 1;

Then there’s a Unit Test that calls these methods. While a Unit Test that executes a hundred thousand lines of this code might get you past your code coverage problem, in my opinion, this is one of the most irresponsible and deceptive things a developer can do. It’s the Potemkin Village of Apex coding. This code almost always results from a developer’s failure to write Unit Tests during the coding process, and it’s amazing how many times, once code this is written, it stays around indefinitely; if the developer couldn’t find the time to write adequate Unit Tests during the project, he or she certainly won’t make the time to replace this code with adequate Unit Tests once the project is over.

Fortunately, I’ve only seen this kind of code a handful of times. Unfortunately, it usually comes hand in hand with many of the other issues described above: undocumented code, non-Bulkified Triggers, hard-coded values, etc. For every “

i = i + 1

,” my blood pressure rises, which is why, even though I don’t see this very often, it still makes my Top Five list of Pet Peeves.

In Conclusion

How can you avoid writing code that has the kinds of problems described above? First, be aware that the code you’re writing will be running in a production org and that the quality and maintainability of your code could be responsible for generating or blocking millions of dollars in productivity and revenue for your company – so it’s worth the time to write that code properly! Secondly, encourage your company’s IT development organization to develop a Coding Standards document, and then conduct your own code reviews (or bring in an expert like OpFocus to conduct your code reviews) to ensure that you’re following those guidelines. What should go into a Coding Standard document? More on that in a future post.
[separator top=”15″ style=”none”]
[tagline_box backgroundcolor=”#eaeaea” shadow=”yes” shadowopacity=”0.1″ border=”1px” bordercolor=”” highlightposition=”top” link=”https://opfocus.com/contact-us” linktarget=”_self” buttoncolor=”blue” button=”Contact Us” title=”Need help from expert developers? OpFocus has a dedicated development team!” animation_type=”slide” animation_direction=”left” animation_speed=”.8″][/tagline_box]
Image credit: http://marcscott.github.io/my_lol_cat/images/Computer_Cat.jpg