Take the 2-minute tour ×
Salesforce Stack Exchange is a question and answer site for Salesforce administrators, implementation experts, developers and anybody in-between. It's 100% free, no registration required.

Here is my situation - I'm fairly new to APEX and programming, currently developing trigger which will do the following:

After update of account details, records in child list should be created/modified based on account picklist field value.

Here is an example:

Picklist contains following values: a,b,c,d,null

When picklist field value changes from 'null' to 'a', a new record in child list should be created. When picklist field value changes from 'a' to 'd', existing record in child list should be modified and new record should be created.

Now, currently I wrote a trigger which handles all situations with if/else statements, like:

for (Account acc : trigger.new) {
    if (System.Trigger.oldMap.get(acc.picklistField) == null && acc.picklistField == 'a') {
        //do all the stuff here
    }
}

Therefore I have a lot of if/else code blocks for every single situation and a lot of code. I feel that it is not the best way to handle situation.

Can anyone provide a review on current solution and maybe propose the way to handle it more efficiently? Thanks!

share|improve this question
    
there is nothing wrong in your approach. because it should be done in that way when you have lot of conditional checks. –  Baskaran S Mar 13 at 11:09
1  
Apex has classes (that can be used from triggers) and interfaces so most of the abstractions and small scale or large scale patterns that can be used in other languages can be used in Apex to create clean code. (Andrew Fawcett's blog andyinthecloud.com describes how some enterprise patterns should be adapted for Salesforce.) Unfortunately though a lot of poor quality code gets posted. –  Keith C Mar 13 at 11:26

3 Answers 3

General Best Practice

There are quite a number of best practices around the Salesforce community on this topic. They all revolve around having as little code in the Apex Trigger as possible, in most cases only one line, and moving the rest into an Apex Class.

Why Apex Classes?

The reason for this is as your starting to discover, an Apex Trigger has no facility for methods, as such it pretty much drives the developer down the age old (and not very object orientated) top-down procedural coding pattern of if/else programming (especially when handling different Apex Trigger events), which starts to get difficult to maintain when things get more complex.

Hence the general best practice is to reach for an Apex Class and split up your logic within methods within it. This then allows you to break your use cases down, making things more readable and maintainable.

Thoughts on your use case...

It sounds like you do need some conditional logic flow at some level (to compare the transition from old and new picklist values), though such logic can be kept reasonably readable if you delegate to an appropriate Apex class method. Keep in mind bulkification when you do...

Set<Id> createList = new Set<Id>();
Set<Id> updateList = new Set<Id>();
for (Account acc : trigger.new) {
    String oldPicklistValue = Trigger.oldMap.get(acc.Id).picklistField__c;
    String newPicklistValue = acc.picklistField__c;
    if (oldPicklistValue == null && newPicklistValue == 'a')
        createList.add(acc.Id);
    else if(oldPicklistValue == 'a' && newPicklistValue == 'd')
        updateList.add(acc.Id);
}
if(createList.size()>0)
    AccountHelper.processCreateList(createList);
if(updateList.size()>0)
    AccountHelper.processUpdateList(updateList);

Thoughts and resources on general Apex Trigger handling...

If you want to explore further there are a number of "Trigger Patterns/Handlers/Wrappers" etc available that help with routing of the various trigger events into Apex class methods and deal with other aspects of Trigger programming for you, such as...

It's worth taking a look at all of them to make your decision on what you feel suites your needs and skills best. They won't neccessarly help with conditionally routing logic to methods based on field value changes, but will help in other aspects of trigger programming you might want to consider generally. Certainly the road your going down is a good one and credit to you for asking this question early on in your development.

Apex Enterprise Patterns

As @Keith C kindly acknowledged, the last one above i'm quite familiar with... The approach used in these patterns derive their name from the those defined by Martin Fowler and provide a Force.com implementation of them. The one your question mostly relates to is the Domain Layer pattern, which is used to encapsulate the behaviour of your objects. You can read more about them and watch some videos via the links on the GitHub site here, there is also a sample application here.

share|improve this answer
1  
You have delivered a fantastic article here @Andrew. Thanks a lot for the additional resources. You're great!! –  Baskaran S Mar 13 at 13:09
1  
Brilliant answer, Andrew, thank you very much! That covered all my needs and beyond. –  Juris Upenieks Mar 13 at 14:06
    
Careful with recursion, a workflow field update on the account will cause this to fire twice. –  Ralph Mar 18 at 21:44

I don't think there is anything wrong with this approach. The only thing I might do is put the logic into a private method to make the code a bit more readable. Something like this

for (Account acc : trigger.new) {
    if (old2new(acc) == 'null2a' {
        //do all the stuff here
    }
    if (old2new(acc) == 'a2d' {
        //do other stuff here
    }
}


private string old2new(Account a){
    if (Trigger.oldMap.get(a.Id).picklistField == null && a.picklistField == 'a') {
        return 'null2A'
    }else if (Trigger.oldMap.get(a.Id).picklistField == 'a' && a.picklistField == 'd') {
        return 'a2d'
    }Else{
        //rest of your logic
    }
}

This is really no different, just an approach to try to help segment the logic and to make it a bit more readable. Your approach is fundamentally the same though.

share|improve this answer

You have to improve your code:

for (Account acc : trigger.new) {
    if (Trigger.oldMap.get(acc.Id).picklistField == null && acc.picklistField == 'a') {
        //do all the stuff here
    }
}

It was little "typo" but it broke code. Trigger.oldMap is Map<Id, SobjectwitholdValues>

Also in case of update you should change your approche from using trigger new to using List<Accounts> get by SOQL thanks to that you will be able to create map of child objects much easier

or just map of child objects - it is your decision :)

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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