0
\$\begingroup\$

I am relatively new in making (invocable)callouts. My main concern is if I should be using smaller parts for example for the body and using a query only once in this case. Any suggestions would be appreciated.

Class:

public class or_service {    
    @InvocableMethod
    public static void InvocePostProperty(List<ID> pIds) {
        for (Id pId : pIds) {
            PostProperty(pId);
        }
    }
    @future(callout=true)
    public static void PostProperty(Id pId) {
        List<Orbirental_Service__c> os = null;
        List<Property__c> prop = null;
        Http http = new Http();
        HttpRequest request = new HttpRequest();
        try {
            os = [SELECT Url__c, ApiKey__c, agencyUid__c FROM Orbirental_Service__c LIMIT 1]; 
            prop = [SELECT id,  Name__c, BaseGuests__c, maximumGuests__c, baseDailyRate__c, city__c, state__c, acceptInstantBook__c, isActive__c ,uid__c
                    FROM Property__c  WHERE id =:pId LIMIT 1];
        }catch(QueryException ex) {
            os = null;
            prop = null;
        }
        request.setEndpoint(os[0].Url__c +'v1/properties/');
        request.setMethod('POST');
        request.setHeader('Content-Type', 'application/json');
        request.setHeader('X-ORBIRENTAL-APIKEY', os[0].ApiKey__c);
        // Set the body as a JSON object
        request.setBody('{"type": "HOUSE"'+
                        ',"name":' + prop[0].Name__c +
                        ',"agencyUid":' + os[0].agencyUid__c +
                        ',"baseGuests":' + prop[0].BaseGuests__c +
                        ',"maximumGuests":' + prop[0].maximumGuests__c +
                        ',"baseDailyRate":' + prop[0].baseDailyRate__c + 
                        ',"city":' + prop[0].city__c + 
                        ',"state":'+ prop[0].state__c + 
                        ',"acceptInstantBook":'+ prop[0].acceptInstantBook__c + 
                        ',"isActive":'+ prop[0].isActive__c+ 
                        '}');
        HttpResponse response = http.send(request);
        // Parse the JSON response
        if (response.getStatusCode() != 200) {
            System.debug('The status code returned was not expected: ' +
                         response.getStatusCode() + ' ' + response.getStatus());
        } else {
            System.debug(response.getBody());
            updateProperty(response.getBody(),pId);
        } 
    } 

    public static void updateProperty(string jsonString, Id pId) {
        List<Property__c> prop = null;        
        or_propertyJSON propClass = or_propertyJSON.parse(jsonString);
        System.debug('myClass.uid '+ propClass.uid);
        try {
            prop = [SELECT id,  Name__c, BaseGuests__c, maximumGuests__c, baseDailyRate__c, city__c, state__c, acceptInstantBook__c, isActive__c ,uid__c
                    FROM Property__c  WHERE id =:pId LIMIT 1];
        }catch(QueryException ex) {
            prop = null;
        }
        prop[0].uid__c = propClass.uid;
        update prop;
    }
}   
\$\endgroup\$
1
\$\begingroup\$

From a practical viewpoint, I think the methods that you have right now are splitting the responsibilities of your overall code fairly appropriately. You could further split things up, but the question to ask is what purpose would this serve?

The more methods and classes you break things into, the more you need to jump around in your class(es) to learn (if you're new to the codebase)/remember (if it has been long enough since you last touched the code) how things work.

Instead of focusing on that, there are more pressing issues:

  • Exceptions as flow control
    • This is generally viewed as an antipattern (i.e. something to avoid). The main purpose of exceptions is to (more) gracefully handle error conditions where continued processing is impossible.
    • In this particular case, running into a System.QueryException should be impossible. In the case where either of your queries returns 0 rows, you can't really continue processing. You'd run into a System.ListException when you try to access os[0] or prop[0]
  • Non-bulkified @future call
    • You're calling your @future method one Id at a time. You can only make 50 @future calls in a given transaction, so if your ProcessBuilder tries to work on 51 records, you're liable to run into this governor limit.
    • The better option here would be to pass the entire list you accept as an argument to your InvocePostProperty() method to your @future method as well
    • Building on the previous suggestion, if possible, you should pass more than one event (I'm assuming it's an event service you're using, based on the language) per callout
  • No notification when async code fails
    • Asynchronous code, at least for me, has a pretty bad history of not printing debugs to logs. The fact that code is being run async also means that there's no direct way to alert you if the async portion of the code fails.
    • In my org, using async code means that we need to do our best to catch all possible errors, and create a Case or send an email to an appropriate person to let them know that a failure has occurred

My Suggestions:

  • I think that Queueable apex is probably more appropriate in your case. Unlike @future methods, queueable apex can be chained ad infinitum
  • Add explicit error notification to your async method(s)
  • Attempt to make more than one callout per async transaction. You can have 100 callouts in a single transaction (as long as your callouts in a given transaction don't collectively exceed 120 seconds of execution time)
  • Make use of the Limits class to determine whether or not you can make another callout in this transaction.
  • Don't forget to store the records you want to update in a list, and perform DML after you finish all of your callouts for a given transaction
  • Instead of trying to catch (and swallow) a System.QueryException, add a custom exception inner class to your or_service class. If you detect 0 rows returned by either of your queries, throw a new instance of this exception (and then do explicit notification)
\$\endgroup\$
  • \$\begingroup\$ Would I need to bulkify my future method if the invocable method is started by a processbuilder? \$\endgroup\$ – Thomas Oct 13 '18 at 5:47
  • \$\begingroup\$ @Thomas I'm afraid I'm not familiar enough with Process Builder to answer that with any certainty. A List of anything, processed one at a time, together with a limited resource (SOQL, DML, callouts) is usually a red flag though. \$\endgroup\$ – Derek F Oct 13 '18 at 12:48

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.