3

My basic question is if it is better to use a try/catch block or an isEmpty check when returning the first element in a list. For example:

return myList.isEmpty() ? null : myList[0];

// OR

try { return myList[0]; }
catch (ListException e) { return null; }

I wrote a utility class to find out the current actor/approver for any given record. It's not currently a singleton or based on lazy-load.

public class CurrentApproverUtil
{
    public static Id findApproverId(Id targetObjId)
    {
        Id actorId;
        for(ProcessInstance pi : [SELECT id, (SELECT ActorId,IsPending FROM StepsAndWorkitems where isPending = true limit 1) step FROM ProcessInstance
                                  where ProcessInstance.TargetObjectId = :targetObjId])
        {
            try
            {
                actorId = pi.StepsAndWorkitems[0].ActorId;
            }
            catch(ListException le)
            {
                //Not calling ExceptionLogging because this method is getting called in a Constructor
            }    
        }
        return actorId;
    }
}

When a record has not entered an approval process the expression pi.StepsAndWorkitems[0] returns NULL and List.Exception is encountered. I handled the exception and I return the NULL. Is it advisable to return null this way instead of using IF,ELSE blocks.

The problem I'm facing is that the object StepsAndWorkItemsis not completely exposed to API.

  • I don't quite understand your code and question here. So you are looping through the ProcessInstance list but only get the value from the last element? – Lance Shi Dec 20 '16 at 7:00
  • @LanceShi I'm looping through ProcessInstance list but I want the element where the approval is pending. That's why the Nested query has a where clause - SELECT ActorId,IsPending FROM StepsAndWorkitems where isPending = true limit 1 – Jarvis Dec 20 '16 at 7:37
  • @LanceShi I'm relying on Try Catch block to return NULL value when no records found. The question is that is it reliable. – Jarvis Dec 20 '16 at 8:07
  • Hey Bennie, I edited your question to bring the more widely applicable problem to the forefront. Obviously, feel free to roll back or further edit if you disagree. – Adrian Larson Dec 20 '16 at 16:38
  • 1
    my selector method for this use case is: return [select id, ProcessDefinitionId, SubmittedById, TargetObjectId, ProcessDefinition.Name, ProcessDefinition.Description, SubmittedBy.Name, SubmittedBy.Email, (select ActorId, Actor.Name, CreatedDate, ElapsedTimeInDays, IsPending, StepStatus // StepStatus is Pending when step is pending; but let caller filter for this if required from StepsAndWorkItems) from ProcessInstance where Status = :STATUS_PENDING ]; } – cropredy Dec 21 '16 at 2:27
5

The main problem with your current try/catch is that an empty catch block is a major anti-pattern. It's not a habit you want to pick up.

In terms of reliability, the behavior of this pattern will be consistent. When you have work items, the first one will be returned. When you have none, null will be returned instead.

In terms of performance, the try/catch strategy is 5.5x - 7.5x slower when the list is empty, but about a third faster when the list is populated. So the exception approach is only faster if you expect a populated list ~ 90% of the time.

I profiled the performance with 20 trials each (1000 iterations), comparing isEmpty vs. catch when the list is empty and when it has an element. I calculate an equilibrium point where the list is empty 11.4% of the time.

Strategy      Empty    Not Empty    Equilibrium
isEmpty     17.90ms      27.90ms         11.36%
catch       98.65ms      17.55ms
  • The column/field IsPending is not present in the table ProcessInstanceWorkItem checked the API. Also tried on Anonymous & Workbench. – Jarvis Dec 20 '16 at 7:55
  • 1
    Ah, I forgot StepsAndWorkItems is a special related list. Well, the second part of the post is still relevant. I'll have to think more on the query...tomorrow. – Adrian Larson Dec 20 '16 at 8:00
  • As you said 'StepsAndWorkItems' is a special related list and isEmpty() is not compiling with this list. – Jarvis Dec 21 '16 at 5:04
  • @Bennie You should be able to compile return pi.StepsAndWorkitems.isEmpty() ? null : pi.StepsAndWorkitems[0]; – Adrian Larson Dec 21 '16 at 5:05
  • 1
    @Bennie - You must have something else going on because if .isEmpty() compiles then it is either a list or a set and since it is from a qury then it is a list and can be accessed via the index. Do you mean that it throws an error instead of "will not compile"? – Eric Dec 21 '16 at 6:36
4

Exceptions are meant to be a rare occurrence, an "exception to the normal flow." I would state that a ListException is always a logic bug in the code, and should not be handled as "business as usual." Adrian makes the point that throwing an exception costs almost 80ms more than simply seeing if the list is empty. That's a pretty significant cost if you expect it to happen more than once every 10 records. There's a lot of overhead with exceptions, so you should avoid using them as normal flow control.

Generally speaking, I recommend that developers do not catch exceptions except in very limited circumstances. There's three classes of exceptions in Apex Code: those that the developer could avoid with simple flow control statements, those that are rare, but anticipated problems, and those that represent an unusual system state where someone needs to be notified. For example, if you're getting a MathException, that means you're doing your math wrong. In your specific case, a ListException is avoided using List.isEmpty. Only for the "rare, but anticipated problem" category should you be catching exceptions.

There are always exceptions to the rule. If you're calling a potentially unavailable web service, you probably want to handle CalloutException, but not JSONException or XMLException. The first is something that is expected and can be dealt with, like trying again later, while the latter two means that the system is giving you gibberish and needs to be fixed, since no amount of retrying is likely to solve the problem.

Of course, this has to be weighed against user inconvenience. If the user is in a 20 page wizard, and page 17 crashes because of an uncaught exception, the user may lose a significant amount of work. Or, if you have an advanced error reporting framework you've created, feel free to catch a broader range of exceptions. My main point here is that every use of try-catch should be considered carefully, and only used when there's no reasonable non-exception way of handling a situation that is rare but known to occur.

  • Fully agree Brian "should not be handled as "business as usual." But what is forcing me here is StepsAndWorkItems is a special related list and isEmpty() on the list is not compiling. – Jarvis Dec 21 '16 at 5:02
  • @Bennie It's not that special; all sub-query lists should automatically be initialized, so something like if(!pi.StepsAndWorkItems.isEmpty()) ... should produce the intended results from your original code. – sfdcfox Dec 21 '16 at 5:13
  • pi.StepsAndWorkitems.isEmpty() compiles but pi.StepsAndWorkitems[0] but doesn't compile – Jarvis Dec 21 '16 at 6:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.