Remote Actions By Any Other Name (Extra Credit: local development?)

While working on a managed package with several developers, dealing with the namespace can be really annoying. Developers (ideally) would not be developing directly in the packaging org, so your team won't have access to the namespace and they'll find themselves trying to figure out how to program more dynamically without hard references to the namespace. This sounds like a good thing, but in practice it can lead to some poor implementation choices, especially when working with Javascript and Remote Actions.

Flawed Dynamic Namespacing

Salesforce recommends using the {!$RemoteAction} merge field to deal with namespacing. For example, your code might look like this:

    Visualforce.remoting.Manager.invokeAction(
'{!$RemoteAction.MyController.doTheThing}',
function(result, event) {
if (event.status) {
handleSuccess();
} else {
handleRejection(event);
}
}
);

This works great because you don't have to worry about the namespace at all. This code will work in your separate developer org as well as the packaging org. However, since static resources don't support merge fields this has to be done within the Visual Force page inside a script block. It's not a huge deal, but you do lose the benefit of the static resources caching plus it makes your Visual Force page messier.

Stick it in an object

Instead just stick your namespaced controller in another variable:

(function(){
'use strict';
var SFDCCtrl = MyNamespace ? MyNamespace.myController : myController

function myRemoteCall(){
SFDCCtrl.doTheThing( function(result, event){
if (event.status) {
handleSuccess();
} else {
handleRejection(event);
}
});
}
}());

In your Visual Force page Salesforce creates global object named after your controller that contains all of your remote actions. If that controller is namespaced, it just wraps that object in an object named after your namespace:

//Namespaced Controller
var MyNamespace = {
MyController : {
doTheThing : function(){};
}
}

//Non-namespaced controller
var MyController = {
doTheThing : function(){};
}

Now you can stick your javascript in a static resource without having to worry about it working in namespace and non-namespaced orgs.

Extra Credit: Possible Local Development?

Local development is like the holy grail of Salesforce programming. I don't think we'll ever have full-fledged local development, but when it comes to building Javascript heavy pages there might be some hope. Here's how I would do the namespacing with Angular:

//SFDCCtrl.js
(function () {
'use strict';
angular.module('myModule')
.service('SFDCCtrl', SFDCCtrl );
function SFDCCtrl(){
this.MyController = MyNamespace ? MyNamespace.MyController : MyController;
}
})();
//AController.js
(function () {
'use strict';
angular.module('myModule')
.controller('AController', AController );
AController.$inject = ['SFDCCtrl'];
function AController(SFDCCtrl){
function myRemoteCall(){
SFDCCtrl.MyController.doTheThing( function(result, event){
if (event.status) {
$scope.theThing = result;
} else {
handleRejection(event);
}
});
}
}
})();

With the angular example, since your controller is tucked away nicely in its own service, you easily mock your SFDC controller with your tests. But why stop there? Why not mock it for development so you can work locally? With Javascript frameworks like Angular and React, I find myself using less Visual Force, simply using the controller for interactions with the database. This can be frustrating because you know that a majority of your page could be compiled on your machine, but you have to hit the server on every save, which can take several minutes for each request! Take the following super simple example that uses the above angular example:

//MyPage.page
<apex:page controller="MyController" applyBodyTag="false">
<body app="myModule">
<div ng-controller="AController">
{{theThing}}
</div>
</body>
<script src="https://ajax.googleapis.com/ajax/libs/angularjs/1.4.2/angular.min.js"></script>
<script src="{!URLFOR($Resource.myAngularApp,'SFDCCtrl.js')}"/>
<script src="{!URLFOR($Resource.myAngularApp,'AController.js')}"/>
</apex:page>

This is basically just a regular html/js page wrapped in visualforce (with a few merge field changes):

<html>
<body app="myModule">
<div ng-controller="AController">
{{theThing}}
</div>
</body>
<script src="https://ajax.googleapis.com/ajax/libs/angularjs/1.4.2/angular.min.js"></script>
<script src="SFDCCtrl.js'"/>
<script src="AController.js'"/>
</html>

Now let's replace the script containing salesforce controller object and replace it with a mock file:

<html>
<body app="myModule">
<div ng-controller="AController">
{{theThing}}
</div>
</body>
<script src="https://ajax.googleapis.com/ajax/libs/angularjs/1.4.2/angular.min.js"></script>
<script src="SFDCCtrlMOCK.js'"/>
<script src="AController.js'"/>
</html>
//SFDCCtrlMOCK.js
(function () {
'use strict';
angular.module('myModule')
.service('SFDCCtrl', SFDCCtrl );
function SFDCCtrl(){
this.MyController = {
doTheThing : doTheThing
}

function doTheThing(){
//return a mock of whatever you expected Salesforce to return you
return 'Athing';
}
}
})();

The Salesforce controller is just an interface now that you or someone else can work on separately from the front end. Now you can work on your page locally! Maintaining these mocks might seem like extra work, but you should probably be building these mocks anyway for Javascript unit testing purposes. Also, the time you save not having to deploy to the server every time you save might even outweigh the cost of building the mock files.

Refactoring: How do I even start?

Let's face it: working with sloppy, legacy code is inevitable. Whether from a rushed developer that's long left the company, or even just you from 6 months ago, eventually you are going to find yourself with big mess of muck that you suddenly need to modify. You have no idea what's going on and there's a bug that cannnot be consistently reproduced. Also, you have until the end of the day and as far as your manager is concerned, refactoring is that thing you learned in middle school to break down numbers.

Also, this guy is your boss.

You are probably a little overwhelmed and feeling pressure from people who don't fully understand the extent of the issue. Maybe you want to build a time machine and stop the original author from doing this (which right now may seem like the easier solution).

It's ok, take a moment to close your eyes and take a deep breath. This too shall pass.

Or scream. That's a good one too.

If you had more time you might be able refactor a lot of the code and maybe implement some design patterns. And if that was the case, check out sourcemaking.com for an extensive list of ideas. But you don't always have that time; sometimes you just need a little help making the code make SOME sense.

Easy ways to clean up code

The key is to start small and stay within a small scope. It'll be easier to keep track of things and staying within a small scope will lessen the chance of side effects. Here are couple of easy, non-invasive techniques to help you make sense of your code and clean it up at the same time

Using and Renaming Variables

Let's say you find a nested for-loop like this:

for( Integer i = 0; i < list1.length(); i++ )
{
for( Integer x = 0; i < list1[i].list2.length(); x++)
{
for( Integer y = 0; i < list1[i].list2[y].list3.length(); y++)
{
if( list1[i].list2[x].list3[y].Name == list1[i].Name && list1[i].list2[x].list3[y].Name == list1[i].list2[x].Name )
{
list1[i].list2[x].list3[y].Name += ' III';
}
}
}
}

You have no idea what these lists are and the chaining is making it difficult to keep track of what is being compared and what is being assigned. But, what we do know is that these lists are related in some kind of hierarchy. So let's rename some of these variables and place repetitive references into new variables

for( Integer i = 0; i < list1.length(); i++ )
{
Object grandparent = grandparent;
List<Object> parents = grandparent.list2;
for( Integer x = 0; i < parents.length(); x++)
{
Object aParent = parents[x];
List<Object> children = aParent.list3;
for( Integer y = 0; i < children.length(); y++)
{
Object aChild = children[y];
if( aChild.Name == grandparent.Name && aChild.Name == aParent.Name )
{
aChild.Name += ' III';
}
}
}
}

A step forward. In our mental map of the code we no longer need to tranverse any hierarchies to figure out what is being operated on and there's a little more context to our variables.

Composing Methods

One of the keys to understanding messy code is reducing the amount of information you need to keep track of in your mental map. Here's the method we worked above with the rest of the method:

public void processList( List<Object> masterList ){
List<Object> list1 = new List<Object>();
for( Integer z = 0; z < masterList.length(); z++ )
{
if( masterList[z].list2 != NULL masterList[z].list2.length() > 0 )
{
for( Integer y = 0; y < masterList[z].list2.length(); y++ )
{
if( masterList[z].list2 != NULL masterList[z].list2.length() > 0 )
{
list1.add(masterList[z]);
}
}
}
}

for( Integer i = 0; i < list1.length(); i++ )
{
Object grandparent = grandparent;
List<Object> parents = grandparent.list2;
for( Integer x = 0; i < parents.length(); x++)
{
Object aParent = parents[x];
List<Object> children = aParent.list3;
for( Integer y = 0; i < children.length(); y++)
{
Object aChild = children[y];
if( aChild.Name == grandparent.Name && aChild.Name == aParent.Name )
{
aChild.Name += ' III';
}
}
}
}
}

After looking at the code for a little bit, you realize that the first for loop is building a list of objects that have both children and grandchildren in its hierarchy. Also you remember that the second for loop is adding a suffix to the grandchildren's name. Instead of having to remember what lines do what, let's move some of this logic out into different methods:

public void processList( List<Object> masterList ){
List<Object> list1 = filterObjectsWithGranchildren( masterList );
appendSuffixToGrandchildren( list1 );
}

private List<Object> filterObjectsWithGranchildren( List<Object> listToFilter ){
List<Object> filteredList = new List<Object>();
for( Integer z = 0; z < listToFilter.length(); z++ )
{
if( listToFilter[z].list2 != NULL listToFilter[z].list2.length() > 0 )
{
for( Integer y = 0; y < listToFilter[z].list2.length(); y++ )
{
if( listToFilter[z].list2 != NULL listToFilter[z].list2.length() > 0 )
{
filteredList.add(listToFilter[z]);
}
}
}
}
return filteredList;
}

private void appendSuffixToGrandchildren( List<Object> grandparents ){
for( Integer i = 0; i < grandparents.length(); i++ )
{
Object grandparent = grandparent;
List<Object> parents = grandparent.list2;
for( Integer x = 0; i < parents.length(); x++)
{
Object aParent = parents[x];
List<Object> children = aParent.list3;
for( Integer y = 0; i < children.length(); y++)
{
Object aChild = children[y];
if( aChild.Name == grandparent.Name && aChild.Name == aParent.Name )
{
aChild.Name += ' III';
}
}
}
}
}

Now our original method calls two private methods. The logic is still the same, but now we can quickly understand what our original method does without having to read through all of the logic.

When it comes to refactoring, this is only the beginning. However, these are my go to strategies when I need to make changes to seemingly nonsensical code and have no idea where to start. Work piece by piece and you'll soon find yourself with a whole.

Eat Your Vegetables! A Lesson in Code Review

Code review can be like eating vegetables. It can be boring, tedious, and sometimes the source of bitterness. Plus the entire time you're tempted to just half-ass it because "Oh I'm sure QA will catch any errors" (you do have a QA process, right?). 

But just like vegetables I think it's safe to say that code review is not only necessary but also good for you. The time required to do is greatly outweighed by the benefits:
  • Increases the Bus Factor of the code base by exposing more team members to code
  • Provides learning opportunities to both the reviewer and reviewee
  • Just the fear of public humiliation discourages developers from cutting corners
Plus there's much more, but that's not really the point here. You know that you need to do code review but how do you even start? Here's a few tips for doing code review with your team:

Choose a Format

Ok, everyone stab as many bugs as you can

Round table session:
A small team gets together and reviews the each of the commits done during the day. The author of the commit explains what's going on in the code while the rest of the teams listens. Generally I think the code should speak for itself, but we were in consulting working on different projects so a little context helped figure out what was trying to be done. However, this can be really disruptive because you basically need to scheduled out an hour or so everyday to have this meeting. Also, some people may be reluctant to speak up at a group meeting.

Gatekeeper:
A designated person (or people) are assigned the task of doing code review for the team. I only recommend this style of coding if you're using outsourced developers and you are having trouble getting them to do code review. You lose a lot of the benefits of code review this way, but it's better than nothing. Also if the gatekeeper is doing any coding then who, uh, gatekeeps(?) the gatekeeper?

Individual Commit Review:
At least two other members of the team individually reviews your code . This allows people to do code review on their own time. However, going through EACH commit can be a little tedious. 

Pull Request Review:
Your pull request is reviewed by at least two individuals. This one is my favorite, as it allows for something cohesive be reviewed as opposed to just chunks of code. However, don't let too much time pass between pull requests or you're going to have MASSIVE amounts of code to review, allowing for great opportunities of things to slip through unnoticed.

Lay Some Ground Rules

The first rule of code review is no fighting.
Fight club is Tuesday after the requirements gathering meeting.
Now that you've chosen a format, make sure to lay some ground rules to help keep code review focused on what's important. Here are a few suggestions:

DO NOT make any personal comments about the author of the code
Nothing constructive comes out of comments like, "This is so dumb, why would you do it like that?" Instead, ask constructive questions like, "Can you restructure this to use X?" This opens the door to a discussion instead of an argument, allowing the author to explain themselves or see the error.

DO NOT take any criticism personally
On the flip side, don't immediately get defensive when someone criticizes your code. The team doesn't don't (or at least shouldn't) care who wrote the code; they just want it to be done well. Take criticism as an opportunity to become a better programmer.

There must be reasoning behind your criticism
Vague complaints like "I don't like it" or "It doesn't feel right" are not valid criticisms. This is not to say you should just stay quiet if something seems awry, but you need to give some direction as to what needs to be done. I can't do anything with just "I don't like it." Also, saying something is not "best practice" is not enough; you need to say why isn't it a best practice. In fact, check out this delightfully cute book on fallacious arguments to help you figure out what's a good argument.

Decide what's in the scope of code review
This is a little more subjective, but your team will need to agree upon what's subject to code review or you may find yourself squabbling over trivial issues. Do you care about whitespace? (you should) Do all web pages need to be mobile friendly? These things come down to establishing coding standards (a whole different discussion), that serve as a (flexible) baseline as to what will be discussed. 

Code Review Checklist

This is definitely not all-inclusive, but it's generally what I look for while doing code reviews (for Salesforce projects)

Triggers
  • Is the trigger bulk safe?
  • Is it safe from recursion?
  • Has the been logic abstracted out of the trigger?
  • Does the trigger use the correct trigger context?
    • e.g. Are updates to non-trigger objects done in the after context?
  • Have all of the trigger contexts been tested?
  • Can the trigger be done with a workflow instead?
Visualforce
  • Have repetitive layouts been abstracted out into a component?
  • Has Javascript/CSS been moved to a public static resource to take advantage of caching?
    • There are exceptions to this such as needing to use merge fields
  • Does the CSS use !important (they shouldn't)
  • Are custom labels used as opposed to hard coded text? (Especially important for multi-language orgs)
Governor Limits
  • Are all SOQL calls outside of loops?
    • Remember to check if methods that contain SOQL are not called in loops
  • Are all future calls outside of loops?
  • Are all callouts outside of loops?
  • Are all DML calls outside of loops?
Tests
  • Do the tests have valid assertions?
  • Have the creation of test records been abstracted out?
  • Are all public methods tested?
Miscellaneous
  • Are all comments necessary and valid?
  • Is the code as modular as possible?
    • If you have to scroll to see what a method does, it's probably too long
  • Have inputs been sanitized as necessary?
    • String.escapeSingleQuotes is your friend!
  • Is everything only as visible as needed?
    • I default to private before making something public
And for our Managed Package friends
  • Are any namespaced items called by a string?
    • Salesforce will change record.MyField__c to record.myNamespace__MyField__c, but it will not be able to pick up record.put('MyField__c', aValue) and you'll get an exception at run time.
  • Are you sure you want that method/class to be global?
  • Are new fields the correct data type?
    • This is always important, but in a Managed Package it's forever.
Again, this is not all inclusive and it should be treated as a guideline, not as a strict rule. There are exceptions to everything and sometimes you have to skip things and take the technical debt in order to get something out in time. 

So get out there and start that code review! Because while it's not always fun to eat your veggies, you'll wished you did when things blowing up and you are all backed up.