Is it a good practice to call a method in my constructor?

Question

I have the following class in which I want to automatically generate the value of premium:

public class MedicalPolicy implements PolicyType {

    private int id;
    private LocalDate effective;
    private LocalDate expiry;
    private String policyNo;
    private double premium;
    private ArrayList<Beneficiary> beneficiaries;

    public MedicalPolicy(LocalDate effective,LocalDate expiry,ArrayList<Beneficiary> beneficiaries){
        this.id=0;
        this.effective=effective;
        this.expiry=expiry;
        this.beneficiaries=beneficiaries;
        this.premium=getPremiumByCalculation();
        this.policyNo= Integer.toString(LocalDate.now().getYear()) + "-Medical-" + Integer.toString(id);
    }

    private double getPremiumByCalculation(){
        //do some calculation based on some criteria
    }
}

Is is a good practice to use a method to calculate the premium like I did or is there a better way?

Maybe you should read this https://stackoverflow.com/questions/18348797/why-is-it-considered-bad-practice-to-call-a-method-from-within-a-constructor — Pedro Estevão, Jan 11 '20 at 01:57
Do whatever leads to the cleanest code. Don't worry about small things like that unless you notice that there's a good reason to worry. — Carcigenicate, Jan 11 '20 at 01:59
And in your case, it's a `private` method, thus not overridable, and thus fine. A `public`, non-`final` method in a non-`final` class would be a different story, however.... — Kevin Anderson, Jan 11 '20 at 02:12

score 2 · Answer 1 · answered Jan 11 '20 at 02:22

No, it's a terrible idea

Don't call methods on partially constructed object. You might think you can work out that it is sufficiently constructed or that if you look in the code you see it'll probably work, but then everyone else will have to go through the deductions and then there's maintenance.

Swing does this lots and it's a mess. Leads to all sorts of NullPointerExceptions.

Sure factor out a method, but don't make it an instance method of the same instance. A particular problem with this code is that it looks like premium needs to be updated with every change, which is going to be very fragile.

score 1 · Answer 2 · answered Jan 11 '20 at 02:34

It's not okay, because fields (even final ones!) may not have the value you expected them to be. A bit of a chicken-and-egg scenario: Your method would presume that the object is fully constructed, and yet, it is not possible to fully construct the object until the method is done.

The usual go-to alternative is static methods. This avoids the entire hullabaloo. This does mean you have to pass in the actual bits of information you need to run this calculation, but that's actually a good thing: It makes it clearer which info is needed, and easier to test.

I notice that your fields aren't final; I assume that the calculation to determine 'premium' depends on at least one of these non-final fields. In that case, have you thought about what happens when that field is updated? Generally, the easiest way to solve this problem is to make all the fields final, and make this an immutable object, neatly sidestepping the 'but what if this is updated?' issue.

A second easy solution is to remove the premium field entirely and recalculate it every single time. If it's all math (some multiplying and application of exponents and logs and the like), well, CPUs are incredibly fast. In fact, a CPU can do 300 to 500 instructions in the time it takes to do a single memory lookup (I'm VASTLY oversimplifying, as a memory lookup of an adjacent field is usually from the same cache page and therefore effectively free, but, hey, if it had to fetch that cache page in the first place, once we're already 500 cycles in, why worry about another 4 cycles, right? Maybe having this object take one double worth of memory less actually saves a cache lookup later, and is thus faster – without profiler reports, don't guess, go for the code that is simpler to write, and getting rid of premium entirely sounds like it'd be simpler here. Have everybody call getPremiumByCalculation() every time (and rename it just getPremium()).

score 0 · Answer 3 · answered Jan 11 '20 at 02:01

0

In my opinion, there is nothing wrong with it. You can test if it works with simple JUnit file, but if it does, then go ahead.

answered Jan 11 '20 at 02:01

Stanley Lapinski

29
7

Is it a good practice to call a method in my constructor?

3 Answers3