Avoid Calling Methods before Initializers in Constructors

Question

We are using Codescan as our static code analyser and for some reason it reports the following Visualforce page controller code has issue with regards of this rule "Avoid calling methods before initializers in constructors".

This is the rule description from CodeScan:

Local variables should be initialized before calling other methods in the class. Since the constructor may not be fully initialized, the methods may fail due to the state of the object. If you need to run a method, try to initialize all variables first. Note that an implemented accessors (e.g. integer a { get { ... return something } }) is counted as a method as it can also rely on other variables

Example from CodeScan:

public class Foo {
  private String status;
  public Foo(String status){
    if ( getStatus() == 'something' ){      //Bad: this would fail, as the constructor has not completely initialized the object yet.
        // ...
    }
    this.status = status;
  }
  public String getStatus(){
    return this.status;
  }
}

So the scanner has flagged the method idFromURL as an issue:

Id opportunityId = idFromURL(URL_PARAM_OPPORTUNITY_ID);

Simplified code for context:

public class MyClass {
    @testVisible
    private static String URL_PARAM_OPPORTUNITY_ID = 'opportunityId';

    public MyClass() {
        Id opportunityId = idFromURL(URL_PARAM_OPPORTUNITY_ID);
        if (opportunityId != null) {
            // load opportunity data here
        }
    }

    private Id idFromURL(String param) {
        String urlRecordId = System.currentPageReference().getParameters().get(param);
        Id recordId;
        try {
            if (urlRecordId != null) {
                recordId = Id.valueOf(urlRecordId);
                return recordId;
            }
        }
        catch (System.StringException ex) {
            ApexPages.addMessage(new ApexPages.Message(ApexPages.Severity.Fatal, 'Invalid ID: ' + urlRecordId));
        }
        return null;
    }
}

The idFromURL method has not referenced any class variable. In the constructor, I also check if the method idFromURL returns a valid ID before proceeding with the next lines of code. I don't think I need to initialise the opportunityId with null before calling the method. I can't quite understand why this is a potential issue. Any thought?

Answer 1

To avoid these situations, redo your controller so the constructor does no work

public MyClass() {}

and make each property use a lazy load getter

public Opportunity oppo {
  get {
    if (oppo == null) {
       String urlRecordId = System.currentPageReference().getParameters().get(URL_PARAM_OPPORTUNITY_ID);
       Opportunity[] oppos = urlRecordId != null
          ? [SELECT ... FROM Opportunity WHERE Id = : urlRecordId]
          : new List<Opportunity>();
       oppo = !oppos.isEmpty() ? oppos[0] : new Opportunity();
    }
    return oppo;
 } set;
}

and deal with errors similarly

public Boolean hasOpportunity {
   get {return this.oppo.id != null;} private set; 
}

and in the VF page:

<apex:pageMessage rendered="{!NOT(hasOpportunity)}" severity="error" title="No Opportunity found. Check URL params"/>