3

It was recommended by a member to begin putting try/catch blocks in my trigger in order to catch Dml and null pointer exceptions. The issue is when I place them it makes my entire program complain. Right now I'm just putting them where I think they would be helpful, but I really don't have a good grasp on it. I have a feeling I'm going to get a few down votes for this but my random placement of the blocks isn't effective..

My test class

@IsTest
public class OverrideTest {

static testmethod void addressOverride() {

    Sampling__c s = new Sampling__c();

    Validation_Region__c region = new Validation_Region__c();
    region.Country_Key__c = 'HK';
    region.Description__c = 'Luxembourg';

    insert region;

    s.Quantity_of_Samples__c = 2;
    s.Override__c = 'Yes';
    s.Country__c = 'hk';
    s.State_Province__c = 'Luxembourg';
    //s.Zip_Postal_Code__c = ''; 
    //s.Zip_Postal_Code__c = ''; 
    //s.Zip_Postal_Code__c = null; 


    try {
        insert S;
    } catch (DmlException e) {
        system.assert(false, e.getMessage() + ' ' + e.getLineNumber());
    }
}
}

So for instance if I was going to add a try/catch on to my for loop like so, it complains about my brackets, which in my experience means I'm missing something simple.

try{
for (Sampling__c s: Trigger.new) {
    if ((s.Country__c != null) && (S.Override__c == 'Yes')) {
        String countryKey = s.Country__c;
        String regionKey = s.State_Province__c;
        //countryKey = countryKey.toUpperCase();
        // regionKey = regionKey.toUpperCase(); This needs to get fixed possibly toUpperCase or change it to .equalsIgnoreCase

        if (validRegions.get(countryKey) != null && validRegions.get(countryKey).get(regionKey) != null) {
            // The Region belongs to the country
            Validation_Region__c vr = validRegions.get(countryKey).get(regionKey);
        } else {
            System.debug('Breaking at Country/Region Validation.');
            break;
        }                   
        }
}catch(Exeception e){//Complains
 System.debug('Stuff broke. '+e.getMessage());//Complains here
}

--

trigger OverrideTrigger on Sampling__c(before insert, before update) {

Set < String > requiresZipCode = new Set < String > {
    'AR', 'AT', 'BE', 'BG', 'BR', 'CA', 'CN', 'CZ', 'DE', 'DK', 'EG', 'ES', 'FI', 'FR', 'GB', 'GR', 'HR', 'HU', 'ID', 'IL', 'IN', 'IR', 'IT', 'JP', 'KR', 'KY', 'KZ', 'MT', 'MU', 'MY', 'NL', 'NO', 'PE', 'PH', 'PL', 'PT', 'RO', 'SE', 'SG', 'SI', 'SK', 'TN', 'TW', 'UA', 'US', 'ZA'
};
Set < String > optionalZipCode = new Set < String > {
    'AU', 'CH', 'CL', 'CO', 'HK', 'IE', 'MX', 'NZ', 'RU', 'TH', 'TR', 'VE'
};

// Top level map is keyed by Country. Inner Map is keyed by Region   

Map < String, Map < String, Validation_Region__c >> validRegions = new Map < String, Map < String, Validation_Region__c >> ();

try{
for (Validation_Region__c objR: [Select Id, Country_Key__c, Description__c, Name FROM Validation_Region__c]) {       
    String countryKey = objR.Country_Key__c;      
    Map < String, Validation_Region__c > regionMap = validRegions.get(countryKey);
    // Maybe rework to use Map.containsKey rather than null check. Would be cleaner.
    if (regionMap == null) {
        regionMap = new Map < String, Validation_Region__c > ();
        validRegions.put(countryKey, regionMap);
    }
    string regionKey = objR.Description__c;
    regionMap.put(regionKey, objR);
}catch(DmlException e){
  System.debug('There has been a DML Exception ' +e.getMessage());
 }

for (Sampling__c s: Trigger.new) {
    if ((s.Country__c != null) && (S.Override__c == 'Yes')) {
        String countryKey = s.Country__c;
        String regionKey = s.State_Province__c;
        //countryKey = countryKey.toUpperCase();
        // regionKey = regionKey.toUpperCase(); This needs to get fixed possibly toUpperCase or change it to .equalsIgnoreCase

        if (validRegions.get(countryKey) != null && validRegions.get(countryKey).get(regionKey) != null) {
            // The Region belongs to the country
            Validation_Region__c vr = validRegions.get(countryKey).get(regionKey);
        } else {
            System.debug('Breaking at Country/Region Validation.');
            break;
        }                
        if ((s.Zip_Postal_Code__c != null) && (requiresZipCode.contains(countryKey))) {
            System.debug('Inside true Requires Zip code condition.');                   
            if ((countryKey == 'HK' || countryKey == 'IE') && (s.Zip_Postal_Code__c.length() > 0)) { //Condition for length of  0!
                System.debug('Inside ' + countryKey + ' this country does not have postal codes.');
                break;
            }
            if ((countryKey == 'TW') && (s.Zip_Postal_Code__c.length() <= 3)) { //Condition for length of  3!
                System.debug('Inside ' + countryKey + ' and its less than 3');
            }
            if ((countryKey == 'AR' || countryKey == 'AT' || countryKey == 'BE' || countryKey == 'CH' || countryKey == 'DK' || countryKey == 'HU' || countryKey == 'NO' || countryKey == 'PH' || countryKey == 'PI' || countryKey == 'SI' || countryKey == 'TN' || countryKey == 'VE' || countryKey == 'ZA' || countryKey == 'CH' || countryKey == 'NZ' || countryKey == 'TN' || countryKey == 'VE') && (s.Zip_Postal_Code__c.length() <= 4)) { //Condition for length of 4!
                System.debug('Inside ' + countryKey + ' that cant be larger than 4.');
            }
            if ((countryKey == 'DE' || countryKey == 'ES' || countryKey == 'FI' || countryKey == 'FR' || countryKey == 'GR' || countryKey == 'HR' || countryKey == 'ID' || countryKey == 'IL' || countryKey == 'IR' || countryKey == 'IT' || countryKey == 'MX' || countryKey == 'MY' || countryKey == 'US' || countryKey == 'MX' || countryKey == 'TH' || countryKey == 'TR') && (s.Zip_Postal_Code__c.length() <= 5)) { //Condition for length of 5!
                System.debug('Inside ' + countryKey + ' and its <= 5!');
            }
            if ((countryKey == 'CA' || countryKey == 'CN' || countryKey == 'CZ' || countryKey == 'IN' || countryKey == 'KZ' || countryKey == 'PL' || countryKey == 'SE' || countryKey == 'SG' || countryKey == 'SK' || countryKey == 'RU' || countryKey == 'CO') && (s.Zip_Postal_Code__c.length() <= 6)) { //Condition for length of 6!
                System.debug('Inside ' + countryKey + ' And its less than 6!');
            }
            if ((countryKey == 'JP' || countryKey == 'KR' || countryKey == 'NL' || countryKey == 'RO' || countryKey == 'AU' || countryKey == 'CL') && (s.Zip_Postal_Code__c.length() <= 7)) { //Condition for length of 7!
                System.debug('Inside ' + countryKey + ' And its less than 7!');
            }
            if ((countryKey == 'GB' || countryKey == 'BR') && (s.Zip_Postal_Code__c.length() <= 9)) { //Condition for length of 9!
                System.debug('Inside ' + countryKey + ' And its less than 9!');
            }
            if ((countryKey == 'US') && (s.Zip_Postal_Code__c.length() <= 10)) { //Condition for length of 9!
                System.debug('Inside ' + countryKey + ' And its less than 10!');
            }
            } 
            else if ((optionalZipCode.contains(countryKey) || (s.Zip_Postal_Code__c == null))) { 
            //////////WORK ON MORE VALIDATION RULES FOR REQUIRED AND OPTIONAL ZipCODES!!! Tomorrow 3/5/2014                                                                      
                    System.debug('Zipcode is null');
            if(!String.isBlank(s.Zip_Postal_Code__c)){   
            if ((countryKey == 'CH' || countryKey == 'NZ' || countryKey == 'TN' || countryKey == 'VE') && (s.Zip_Postal_Code__c.length() == 4)) { //Condition for length of  4!
                System.debug('Inside ' + countryKey + ' this zip code matches');
            }
            if ((countryKey == 'MX' || countryKey == 'TH' || countryKey == 'TR') && (s.Zip_Postal_Code__c.length() == 5)) { //Condition for length of  5!
                System.debug('Inside ' + countryKey + ' this zip code matches');
            }
            if ((countryKey == 'RU' || countryKey == 'CO') && (s.Zip_Postal_Code__c.length() == 6)) { //Condition for length of  6!
                System.debug('Inside ' + countryKey + ' this zip code matches');
            }
            if ((countryKey == 'AU' || countryKey == 'CL') && (s.Zip_Postal_Code__c.length() <= 7)) { //Condition for length of  7!
                System.debug('Inside ' + countryKey + ' this zip code matches');               
          } else {
            System.debug('The countryCode ' + countryKey + 's' + ' zipcode is not the proper length. There is an error inside the Zipcode Field');     
            break;       
          } 
        }
      } 
    } 
  }
}
12
  • I amy be missing something, where in the trigger are you doing DML itself?
    – joshbirk
    Mar 5, 2014 at 18:44
  • 1
    Try changing the catch to Exception, instead of DMLException. You don't have any DML statements, so you won't catch any exceptions of that type
    – kbentsen
    Mar 5, 2014 at 18:50
  • 1
    Odd. Try putting the catch block on a new line after the closing bracket of the try block.
    – kbentsen
    Mar 5, 2014 at 19:07
  • 1
    I was actually suggesting moving the logic from the trigger into a class that you call from the trigger and having the test invoke that class. That would allow you to improve the structure of the code by factoring it out into multiple methods and by making more use of sets for the collections of values (though that could be done in the trigger too).
    – Keith C
    Mar 5, 2014 at 19:19
  • 1
    Just remove all of those try/catch blocks. None of those places are useful to have them at. This might help you understand them more: salesforce.com/us/developer/docs/apex_workbook/Content/…
    – dphil
    Mar 5, 2014 at 20:50

3 Answers 3

8

Sometimes, it makes sense to try/catch, but not always. Some code will catch the exception: it might not be your code. You need to ask yourself a few things.

Can my code recover from the exception? If you can catch the exception and do something useful, then by all means, catch the exception and recover from the error.

Does the exception really matter? Sometimes, especially when invoking library/framework code, you might need to catch and eat (ignore) an exception that does not affect the overall processing of your program.

Can the exception be thrown? Does it make sense to catch a divide by zero exception if you already checked the divisor against zero and avoided dividing? Does it make sense to do something with a null pointer exception if you already know the variable cannot be null?

Can my code do something useful with the exception? If the code in question is low-level it might not make sense to do anything with the exception. Let it bubble higher up the call stack to another process that can actually do something useful with it.

There is no one-size-fits-all approach, but surrounding an entire method with try/catch is normally a code smell. It sounds like the advice you received was out of laziness: catch all exceptions so it does not fail. But those failures can be important: a null pointer or DML exception is valuable information that can direct you toward a logic error in your code so you can fix the bug. Eating that exception just makes your code more difficult to prove correct.

3

Use try / catch in unit tests to test for expected DML outcomes

In your case:

try {
    insert S;
} catch (DmlException e) {
    system.assert(false, e.getMessage() + ' ' + e.getLineNumber());
}

Is making the assumption that the test data will fail the insert. And you probably want something more like:

try {
    insert S;
} catch (DmlException e) {
    system.assertEquals(true, e.getMessage().contains('my expected error');
}

The reason to use try / catch here is that the unit test can compile and run without triggering a system error but will still properly assert the outcome of the error (and that it is the right error). Using it your normal class will, as you've got it, allow you to control the debugging.

Specifically in your case, the bracket error is not due technically to the use of try/catch, you're just missing a bracket towards the end:

}//add bracket here
} catch(Exeception e) {//Complains
 System.debug('Stuff broke. '+e.getMessage());//Complains here
}
3
  • I think he just generally doesn't understand the purpose of try/catch blocks. He doesn't appear to actual expect it to fail... I just think someone told him he should have some somewhere, and so placed them all over.
    – dphil
    Mar 5, 2014 at 20:52
  • dphil, hit it.. I'm not expecting it to fail and I'm trying to test failures right now through my test class, and thought have try/catch blocks would help any future issues
    – EricSSH
    Mar 5, 2014 at 21:22
  • In your main class, it might help stability in the long run. In a unit test - it's more specific. Especially with an assert there.
    – joshbirk
    Mar 5, 2014 at 22:02
3

It was recommended by a member to begin putting try/catch blocks in my trigger in order to catch Dml and null pointer exceptions.

  • Try-catch within before triggers won't catch any DML errors on the implicit DML done on Trigger.new as the DML occurs after the trigger executes and SFDC will take care of rolling back the transaction and surfacing an error
  • Try-catch or the equivalent Database.dmloperation within after triggers if such after triggers are doing DML is useful because there may be things you want to do - support partial updates, do full or partial rollbacks, log the error, and/or issue a more useful error message that has more context helping your user or sysad debug the problem
  • Try-catch within VF controller action methods, especially if they do DML, is useful for the same reasons as above
  • Sometimes, I'll use try-catch blocks when invoking services not under my control - to ensure that if an error occurs, I do the proper action and surface a useful error message.

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .