As a technical lead, I 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. Here are a few of my own best practices for 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!
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.
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 to ensure that you’re following those guidelines. What should go
into a Coding Standard document? More on that in a future post.
