5

Is it possible to rewrite this:

public Assessment__c thisAssessment;

    public Assessment__c getthisAssessment() {
        if (assessmentUrlId!=null) {
            return thisAssessment=[SELECT Id, name
                                    FROM Assessment__c 
                                    WHERE Id = :assessmentUrlId];
        }
        else {
            return new Assessment__c();
        }       
    }       

Into this style notation? For some reason the setter doesn't seem to work when I write it this way.

public Assessment__c thisAssessment {
        get {
            if (assessmentUrlId!=null) {
                return [SELECT Id,name 
                                        FROM Assessment__c 
                                        WHERE Id = :assessmentUrlId];
            }
            else {
                return new Assessment__c();
            }  
        }
        set;
    }
9

I'd suggest that you simply don't provide a setter implementation. Just leave it out unless you need it to do something specifically, like set the assessmentUrlId.


Alternatively, you will need to provide an implementation for the setter.

This example uses the implicit property storage for values. An explicit member isn't created to store the value like you would otherwise do in C#.

private Id assessmentUrlId;

public Assessment__c thisAssessment {
    get {
        if(thisAssessment == null) {
            // Typically a getter shouldn't change the objects state.
            if (assessmentUrlId != null) {
                // Use this with caution!
                // You don't want to be doing the same SOQL query every time
                // something accesses this property.
                thisAssessment = [SELECT Id,name 
                                      FROM Assessment__c WHERE Id = :assessmentUrlId];
            } else {
                thisAssessment = new Assessment__c();
                // Should the name be defaulted to something in the new instance?
                // At what point will the new Assessment be inserted?
            }
        }            
        return thisAssessment;
    }
    set {
        thisAssessment = value;

        // Do you want to insert or update any assigned value to the database?
        // I wouldn't recommend this as good practice in a property. 
        // Better that the caller has already inserted it or it is inserted later.
        //upsert value;

        assessmentUrlId = value.Id;
    }
}

With regards to the getter method. Refer to this note from the Force.com Apex Code Developer's Guide on Apex Properties:

We recommend that your get accessor should not change the state of the object that it is defined on.

Instead of doing the SOQL query to retrieve the existing assessment in the Property you could initialize it in the controllers/objects constructor.

  • 2
    Glad you updated that Daniel, a set implementation doing DML was giving me heart palpitations! ;-) – Andrew Fawcett Nov 15 '12 at 22:57
  • I got there in the end :-) with my edits. It isn't very clear what a setter should do in this case. I'd probably leave it out altogether, or as you suggest, stick with a method as it may be creating a new empty record. – Daniel Ballinger Nov 15 '12 at 23:26
  • Thanks! One thing I can't wrap my head around is why do you need the if(thisAssessment == null) check? Shouldn't it only be calling the getter once? – Phil B Nov 16 '12 at 2:08
  • @turbo2oh That depends on how many places the property is referenced from. If it is only called once from a Visualforce page or apex, then yes, it would only run once. I figured it would be safer to be more defensive and only do the relatively expensive SOQL query if required. – Daniel Ballinger Nov 16 '12 at 2:14
  • @DanielBallinger sorry one last thing, why is the ==null check not needed in the getthisAssessment() version in my first example? It seems to work correctly without it (not getting overwritten). – Phil B Nov 16 '12 at 2:26
4

It is because your get and set implementations are not balanced. Your get method never actually returns the value of the 'thisAssessment' property your defining. It either returns a queried instance or a new instance. So you can assign it as long as you like, the 'get' will continue to mask the actual value.

I'm not a big fan of set/get methods with side effects nor ones with extensive implementation in them. IMHO they should primarily be used for data access and that's it. Daniels code above gives a good way to fix it up if you want to continue this route.

Personally I like your original method based version better, it is a clearer statement of intent it being a method, that it has behaviour. Doing SOQL in a get is just waiting for a bulkifcation bug as developers are unaware that a simple .thisAssement is doing so much. And as such implementing this way, feels like its giving the wrong impression, as it is not just a data accessor / mutator as most get/set implementations are typically expected to be.

If you search for 'Properties vs Methods' via Google, you'll find some good reading on the topic. Mostly .Net, since it was the first to add this feature (I think?). But you'll get the general idea.

  • I just learnt that Apex properties have implicit members from your previous code example. Thanks! – Daniel Ballinger Nov 16 '12 at 0:07
  • Hmm interesting I'll def. have to read up some. In the original method based version wouldn't a simple thisAssessment call the get method and present the same challenge? – Phil B Nov 16 '12 at 3:00
  • It comes down to perception and assumption of your caller / developer, your right in the fact that Assement__c assement = myvar.getThisAssement() vs Assement__c assement = myvar.thisAssement may not be a million miles appart, but one does give rise to more happening behind the scenes than the other. And in this case neither should really be doing SOQL to be honest, as Daniel has pointed out. – Andrew Fawcett Nov 16 '12 at 11:05
  • Thanks, after all this I ended up moving the logic/query into the constructor anyway, just trying to make sure I understood what was going on. – Phil B Nov 16 '12 at 14:44
  • :) Glad you got it sorted, I do think it is better placed there. Nice discussion though! +1 on the question from me! – Andrew Fawcett Nov 16 '12 at 14:51

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.