26

Accessors and modifiers (aka setters and getters) are useful for three main reasons:

  1. They restrict access to the variables.
    • For example, a variable could be accessed, but not modified.
  2. They validate the parameters.
  3. They may cause some side effects.

Universities, online courses, tutorials, blog articles, and code examples on the web are all stressing about the importance of the accessors and modifiers, they almost feel like a "must have" for the code nowadays. So one can find them even when they don't provide any additional value, like the code below.

public class Cat {
    private int age;

    public int getAge() {
        return this.age;
    }

    public void setAge(int age) {
        this.age = age;
    }
}

That been said, it is very common to find more useful modifiers, those which actually validate the parameters and throw an exception or return a boolean if invalid input has been supplied, something like this:

/**
 * Sets the age for the current cat
 * @param age an integer with the valid values between 0 and 25
 * @return true if value has been assigned and false if the parameter is invalid
 */
public boolean setAge(int age) {
    //Validate your parameters, valid age for a cat is between 0 and 25 years
    if(age > 0 && age < 25) {
        this.age = age;
        return true;
    }
    return false;
}

But even then, I almost never see the modifiers been called from a constructor, so the most common example of a simple class I face with is this:

public class Cat {
    private int age;

    public Cat(int age) {
        this.age = age;
    }

    public int getAge() {
        return this.age;
    }

    /**
     * Sets the age for the current cat
     * @param age an integer with the valid values between 0 and 25
     * @return true if value has been assigned and false if the parameter is invalid
     */
    public boolean setAge(int age) {
        //Validate your parameters, valid age for a cat is between 0 and 25 years
        if(age > 0 && age < 25) {
            this.age = age;
            return true;
        }
        return false;
    }

}

But one would think that this second approach is a lot safer:

public class Cat {
    private int age;

    public Cat(int age) {
        //Use the modifier instead of assigning the value directly.
        setAge(age);
    }

    public int getAge() {
        return this.age;
    }

    /**
     * Sets the age for the current cat
     * @param age an integer with the valid values between 0 and 25
     * @return true if value has been assigned and false if the parameter is invalid
     */
    public boolean setAge(int age) {
        //Validate your parameters, valid age for a cat is between 0 and 25 years
        if(age > 0 && age < 25) {
            this.age = age;
            return true;
        }
        return false;
    }

}

Do you see a similar pattern in your experience or is it just me being unlucky? And if you do, then what do you think is causing that? Is there an obvious disadvantage for using modifiers from the constructors or are they just considered to be safer? Is it something else?

jskroch
  • 109
  • 3
Allan Spreys
  • 845
  • 1
  • 8
  • 13
  • It is highly critical to use methods that can be overridden in the constructor. – stg Aug 31 '16 at 11:47
  • 1
    @stg, thank you, interesting point! Could you expand on it and put it into an answer with an example of a bad thing which would happen maybe? – Allan Spreys Aug 31 '16 at 11:50
  • 1
    possible duplicate of [Should the methods of a class call its own getters and setters?](http://programmers.stackexchange.com/questions/181567/should-the-methods-of-a-class-call-its-own-getters-and-setters) – gnat Aug 31 '16 at 11:57
  • 8
    @stg Actually, it is an anti-pattern to use overridable methods in the constructor and many code quality tools will point it out as an error. You don't know what the overridden version will do and it might do nasty things like letting "this" escape before the constructor finishes which can lead to all sort of strange issues. – Michał Kosmulski Aug 31 '16 at 13:15
  • 1
    @gnat: I don't believe this is a duplicate of [Should the methods of a class call its own getters and setters?](http://programmers.stackexchange.com/questions/181567/should-the-methods-of-a-class-call-its-own-getters-and-setters), because of the nature of constructor calls being invoked on an object that is not fully formed. – Greg Burghardt Aug 31 '16 at 13:50
  • 3
    Note also that your "validation" just leaves age un-initialized (or zero, or ... something else, depending on language). If you want the constructor to fail, you have to check the return value and throw an exception on failure. As it stands, your validation just introduced a different bug. – Useless Aug 31 '16 at 15:47
  • @stg: that could be easily fixed by making the setter final. Still the OP's question remains. – Doc Brown Aug 31 '16 at 18:10
  • @DocBrown Unless that final setter calls a non-final method. – JimmyJames Aug 31 '16 at 18:19
  • @JimmyJames: the question is not "can this be implemented in buggy way" - surely it can. The question is to my understanding "why is a constraint checked in a setter, but omitted in the ctor? And the correct answer is: it should **not** be omitted (opposed to Caleb's answer), but implemented correctly (by not using the setter directly in the ctor, but a separate check method, as shown in Useless' answer). – Doc Brown Aug 31 '16 at 20:17
  • @DocBrown I guess we don't see the question the same way. I'm answering the question "why don't we call setters in constructors despite the apparent benefits." The answer is probably related to how things can go wrong. My response to your comment was meant to highlight that there are lots of ways it can and why it's probably best avoided. – JimmyJames Aug 31 '16 at 20:31
  • @JimmyJames: I posted an answer to make more clear what probably causes here the confusion. – Doc Brown Aug 31 '16 at 21:49
  • 1
    Where does the baker get his own bread from: Does he use the *official interface* (i.e. his shop) and buy it from (maybe) his wife over the counter? Or does he use no interface at all with direct access (i.e. his own oven)? – tofro Sep 09 '16 at 16:02
  • @tofro while I find your example amazing, I don't think it is accurate. The class (baker) cannot guarantee what supplies (parameters) she may receive from her suppliers. They may send her flower or they may send her sand instead. It is her job to validate the supplies before she bakes from them to make sure that her customers are happy. – Allan Spreys Sep 09 '16 at 18:27
  • 1
    Your "useless" getters and setters are _excellent_ for debugging. Just leave a breakpoint on that return, and you can find out who is calling that thing via Stack Trace. Really useful for event-based apps! – T. Sar Sep 09 '16 at 18:37

5 Answers5

41

Very general philosophical reasoning

Typically, we ask that a constructor provide (as post-conditions) some guarantees about the state of the constructed object.

Typically, we also expect that instance methods can assume (as pre-conditions) that these guarantees already hold when they're called, and they only have to make sure not to break them.

Calling an instance method from inside the constructor means some or all of those guarantees may not yet have been established, which makes it hard to reason about whether the instance method's pre-conditions are satisfied. Even if you get it right, it can be very fragile in the face of, eg. re-ordering instance method calls or other operations.

Languages also vary in how they resolve calls to instance methods which are inherited from base classes/overridden by sub-classes, while the constructor is still running. This adds another layer of complexity.

Specific examples

  1. Your own example of how you think this should look is itself wrong:

    public Cat(int age) {
        //Use the modifier instead of assigning the value directly.
        setAge(age);
    }
    

    this doesn't check the return value from setAge. Apparently calling the setter is no guarantee of correctness after all.

  2. Very easy mistakes like depending on initialization order, such as:

    class Cat {
      private Logger m_log;
      private int m_age;
    
      public void setAge(int age) {
        // FIXME temporary debug logging
        m_log.write("=== DEBUG: setting age ===");
        m_age = age;
      }
    
      public Cat(int age, Logger log) {
        setAge(age);
        m_log = log;
      }
    };
    

    where my temporary logging broke everything. Whoops!

There are also languages like C++ where calling a setter from the constructor means a wasted default initialization (which for some member variables at least is worth avoiding)

A simple proposal

It's true that most code isn't written like this, but if you want to keep your constructor clean and predictable, and still reuse your pre- and post-condition logic, the better solution is:

class Cat {
  private int m_age;
  private static void checkAge(int age) {
    if (age > 25) throw CatTooOldError(age);
  }

  public void setAge(int age) {
    checkAge(age);
    m_age = age;
  }

  public Cat(int age) {
    checkAge(age);
    m_age = age;
  }
};

or even better, if possible: encode the constraint in the property type, and have it validate its own value on assignment:

class Cat {
  private Constrained<int, 25> m_age;

  public void setAge(int age) {
    m_age = age;
  }

  public Cat(int age) {
    m_age = age;
  }
};

And finally, for complete completeness, a self-validating wrapper in C++. Note that although it's still doing the tedious validation, because this class does nothing else, it's relatively easy to check

template <typename T, T MAX, T MIN=T{}>
class Constrained {
    T val_;
    static T validate(T v) {
        if (MIN <= v && v <= MAX) return v;
        throw std::runtime_error("oops");
    }
public:
    Constrained() : val_(MIN) {}
    explicit Constrained(T v) : val_(validate(v)) {}

    Constrained& operator= (T v) { val_ = validate(v); return *this; }
    operator T() { return val_; }
};

OK, it isn't really complete, I've left out various copy and move constructors and assignments.

Deduplicator
  • 8,591
  • 5
  • 31
  • 50
Useless
  • 12,380
  • 2
  • 34
  • 46
  • 4
    Heavily undervoted answer! Let me add just because the OP has seen the described situation in a text book does not make it a good solution. If there are two ways in a class to set an attribute (by constructor and setter), and one wants to enforce a constraint on that attribute, all both ways must check the constraint, otherwise it is a **design bug**. – Doc Brown Aug 31 '16 at 18:18
  • So would you consider using setter methods in a constructor bad practice **if** there is 1) no logic in the setters, or 2) the logic in the setters is independent of state? I dont do it often but I personally have used setter methods in a constructor exactly for the latter reason (when there is some type of condition on in setter that must always apply to the member variable – Chris Maggiulli Mar 10 '19 at 02:46
  • If there is some type of condition that must always apply, that _is_ logic in the setter. I certainly think it is better _if possible_ to encode this logic in the member, so it is forced to be self-contained, and cannot acquire dependencies on the rest of the class. – Useless Mar 10 '19 at 18:03
  • But doesn't not using setter in constructor mean we also lose whatever structure and logic we place in the setter? So the initially instantiated object has a potential to be invalid. E.g. say a class' setter receives a string and formats it, by instead passing the same string to constructor and set directly on property means it doesn't get the same formatting. Not saying you are incorrect, just throwing the idea out there. – James Sep 22 '22 at 10:48
  • An initially instantiated object is _by definition_ invalid until it's initialized, and initialization is the responsibility of the constructor. You're positing a setter which has no preconditions (and is sufficient to establish the constructor's postconditions), so in that case calling the setter would be fine. But this is not the _general_ case, so we don't recommend calling setters from the constructor _in general_. – Useless Sep 22 '22 at 12:09
13

When an object is being constructed, it is by definition not fully formed. Consider how the setter you provided:

public boolean setAge(int age) {
    //Validate your parameters, valid age for a cat is between 0 and 25 years
    if(age > 0 && age < 25) {
        this.age = age;
        return true;
    }
    return false;
}

What if part of the validation in setAge() included a check against the age property to ensure that the object could only increase in age? That condition might look like:

if (age > this.age && age < 25) {
    //...

That seems like a pretty innocent change, since cats can only move through time in one direction. The only problem is that you can't use it to set the initial value of this.age because it assumes that this.age already has a valid value.

Avoiding setters in constructors makes it clear that the only thing the constructor is doing is setting the instance variable and skipping any other behavior that happens in the setter.

Caleb
  • 38,959
  • 8
  • 94
  • 152
  • OK ... but, if you don't actually test object construction and expose the potential bugs, there are potential bugs *either way*. And now you've got two *two* problems: You have that hidden potential bug *and* you have to enforce age-range checks in two places. – svidgen Aug 31 '16 at 14:25
  • There is no problem, as in Java/C# all fields are zeroed before constructor-invocation. – Deduplicator Aug 31 '16 at 14:50
  • Different languages deal with these issues in different ways, e.g. C++ has member initializers while Objective-C recommends that you just do as little work as possible in your initialization method. This question is language-agnostic and just focuses on the general pattern, and the general answer is that you want to make sure that the object is created properly before you start to use it. Even using a class's own setter is asking for trouble, and it only gets worse if the setter can be overridden. – Caleb Aug 31 '16 at 15:22
  • I have a really hard time believing it's the "general answer" ... It's certainly not what I was taught! Nor is it what I've seen in practice... – svidgen Aug 31 '16 at 15:42
  • 2
    Yes, calling instance methods from constructors can be hard to reason about, and is not generally preferred. Now, if you want to move your validation logic into a private, final class/static method, and call it from both the constructor and setter, that would be fine. – Useless Aug 31 '16 at 15:45
  • @Useless Except, you're still basically calling your setter logic then. And now you're *also* in denial! – svidgen Aug 31 '16 at 15:49
  • 1
    No, non-instance methods avoid the trickiness of instance methods, because they don't touch a partly-constructed instance. You do, of course, still need to check the result of validation to decide whether construction succeeded. – Useless Aug 31 '16 at 15:50
  • 1
    This answer is IMHO missing the point. *"When an object is being constructed, it is by definition not fully formed"* - in the middle of the construction process, of course, but when the constructor is done, any intendended constraints on attributes must hold, otherwise one can circumvent that constraint. I would call that a severe design flaw. – Doc Brown Aug 31 '16 at 18:22
  • @Deduplicator: Java/C# zero-initialization still has this problem if zero isn't a *valid* value for the property in question. – dan04 Aug 31 '16 at 23:27
6

Some good answers here already, but noone so far seemed to have noticed you are asking here two things in one, which causes some confusion. IMHO your post is best seen as two separate questions:

  1. if there is a validation of a constraint in a setter, should it not also be in the constructor of the class to make sure it cannot be bypassed?

  2. why not call the setter directly for this purpose from the constructor?

The answer to first questions is clearly "yes". A constructor, when it does not throw an exception, should leave the object in a valid state, and if certain values are forbidden for certain attributes, then it makes absolutely no sense to let the ctor circumvent this.

However, the answer to the second question is typically "no", as long as you do not take measures to avoid overriding the setter in a derived class. Therefore, the better alternative is to implement the constraint validation in a private method which can be reused from the setter as well as from the constructor. I am not going here to repeat @Useless example, which shows exactly this design.

Doc Brown
  • 199,015
  • 33
  • 367
  • 565
  • I still don't understand why it's better to extract logic from the setter so that the constructor can use it. ... How is this *really* different than simply calling the setters in a reasonable order?? Especially considering that, if your setters are as simple as they "should" be, the only part of the setter your constructor, at this point, is *not* invoking is the actual setting of the underlying field ... – svidgen Aug 31 '16 at 21:47
  • 2
    @svidgen: see JimmyJames example: in short, it is not a good idea to use overridable methods in a ctor, that will cause problems. You can use the setter in ctor if it is final and does not call any other overridable methods. Moreover, separating the validation from the way to handle it when it fails offers you the opportunity to handle it differently in the ctor and in the setter. – Doc Brown Aug 31 '16 at 21:52
  • Well, I'm even less convinced now! But, thanks for pointing me to the other answer ... – svidgen Aug 31 '16 at 22:09
  • 2
    By _in a reasonable order_ you mean _with a brittle, invisible ordering dependency easily broken by innocent-looking changes_, right? – Useless Aug 31 '16 at 23:02
  • @useless well, no... I mean, it's funny that you'd suggest the order in which your code executes shouldn't matter. but, that wasn't really the point. Beyond contrived examples, I just can't recall an instance in my experience where I've thought it was a good idea to have cross-property validations in a setter--that sort of thing leads *necessarily* to "invisible" invocation-order requirements. Regardless of whether those invocations occur in a constructor.. – svidgen Sep 01 '16 at 00:52
2

Here's a silly bit of Java code that demonstrates the kinds of issues you can run into with using non-final methods in your constructor:

import java.util.regex.Pattern;

public class Problem
{
  public static final void main(String... args)
  {
    Problem problem = new Problem("John Smith");
    Problem next = new NextProblem("John Smith", " ");

    System.out.println(problem.getName());
    System.out.println(next.getName());
  }

  private String name;

  Problem(String name)
  {
    setName(name);
  }

  void setName(String name)
  {
    this.name = name;
  }

  String getName()
  {
    return name;
  }
}

class NextProblem extends Problem
{
  private String firstName;
  private String lastName;
  private final String delimiter;

  NextProblem(String name, String delimiter)
  {
    super(name);

    this.delimiter = delimiter;
  }

  void setName(String name)
  {
    String[] parts = name.split(Pattern.quote(delimiter));

    this.firstName = parts[0];
    this.lastName = parts[1];
  }

  String getName()
  {
    return firstName + " " + lastName;
  }
}

When I run this, I get a NPE in the NextProblem constructor. This is a trivial example of course but things can get complicated quickly if you have multiple levels of inheritance.

I think the bigger reason this didn't become common is that it makes the code a good bit harder to understand. Personally, I almost never have setter methods and my member variables are almost invariably (pun intended) final. Because of that, the values have to be set in the constructor. If you use immutable objects (and there are many good reasons to do so) the question is moot.

In any event, it's a good goal to reuse the validation or other logic and you can put it into a static method and invoke it from both the constructor and the setter.

JimmyJames
  • 24,682
  • 2
  • 50
  • 92
  • How is this at all a problem of using setters in the constructor instead of a problem of poorly implemented inheritance??? In other words, if you're effectively invalidating the base constructor, *why would you even call it!?* – svidgen Aug 31 '16 at 22:08
  • 1
    I think the point is, that overriding a setter, shouldn't invalidate the constructor. – Chris Wohlert Sep 01 '16 at 13:16
  • @ChrisWohlert Ok ... but, if you change your setter logic to conflict with your constructor logic, it's a problem regardless of whether your constructor uses the setter. Either it fails at construction time, it fails later, or it fails *invisibly* (b/c you've bypassed your business rules). – svidgen Sep 01 '16 at 13:44
  • You often don't know how the Constructor of the base-class is implemented (it might be in a 3rd party library somewhere). Overriding an overridable method should not break the contract of the base-implementation, though (and arguably this example does so by not setting the `name` field). – Hulk Sep 01 '16 at 13:44
  • @svidgen the problem here is that calling overridable methods from the constructor reduces the usefulness of overriding them - you cannot use any fields of the child class in the overridden versions because they might not be initialized yet. What is worse, you must not pass a `this`-reference to some other method, because you can't be sure its fully initialized yet. – Hulk Sep 01 '16 at 13:54
  • @Hulk Sorry, but I'm still not convinced. If you're your in the habit of extending classes, you need to make sure your overrides are compatible with the rest of the class *anyway!* (including the constructor, mind you.) Furthermore, if you're overriding a setter, what's the value in overriding at all if your nothing actually uses it? ... In other words, if you're class is typically created through a constructor that *doesn't* use the setters, you're sort of wasting your time by overriding them. Either that, or you have to override them *and* the constructor. – svidgen Sep 01 '16 at 13:59
  • And as others have suggested, you can move the validation logic to another method and call it from both the setter *and* the getter. But, it's the same problem if you leave that method open for extension. And, if you don't ... well ... if you're not leaving things open for extension *anyway*, the whole point is moot. Just use the damn setters -- no one can override them. That said, if someone can show an *realistic* example demonstrating a real problem with calling setters in a constructor, rather than poor design *in general*, that'd be cool, b/c this weird dogma sounds new and nutty to me. – svidgen Sep 01 '16 at 14:01
  • @svidgen My point is that - at least in Java - there is no safe way to use overridable methods from a constructor. (with "safe" I mean without requiring the implementer of the Child class to inspect the source code of the Base class and only using fields that are already initialized before the first invocation). – Hulk Sep 01 '16 at 14:11
  • @Hulk But, if that's true (that it's simply not safe), that applies to *all* overridable methods. Including any overridable validation logic. So, the answers here are still totally missing the point! ... are they not? ... Maybe I just need to simmer on this for awhile. But, the examples here still all seem totally contrived -- examples of awful design; not the woes of using setter methods in a constructor... – svidgen Sep 01 '16 at 14:13
  • Yes, this applies to all overridable methods. – Hulk Sep 01 '16 at 14:19
1

It depends!

If you're performing simple validation or issuing naughty side-effects in the setter that need to be hit every time the property is set: use the setter. The alternative is generally to extract the setter logic from the setter and invoke the new method followed by the actual set from both the setter and the constructor -- which is effectively the same as using the setter! (Except now you're maintaining your, admittedly small, two-line setter in two places.)

However, if you need to perform complex operations to arrange the object before a particular setter (or two!) could successfully execute, don't use the setter.

And in either case, have test cases. Regardless of which route you go, if you have unit tests, you'll have a good chance of exposing the pitfalls associated with either option.


I'll also add, if my memory from that far back is even remotely accurate, this is the sort of reasoning I was taught in college too. And, it's not at all out of line with successful projects I've had the privy to work on.

If I had to guess, I missed a "clean code" memo at some point that identified some typical bugs can that arise from using setters in a constructor. But, for my part, I haven't been victim to any such bugs ...

So, I'd argue that this particular decision doesn't need to be sweeping and dogmatic.

svidgen
  • 13,414
  • 2
  • 34
  • 60