23

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?

  • 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? – Vlad Spreys Aug 31 '16 at 11:50
  • 1
    possible duplicate of 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?, 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
37

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.

  • 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 at 2: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 at 18:03
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.

  • 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
  • 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
  • 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
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.

  • 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 1 '16 at 0: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.

  • 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 1 '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 1 '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 1 '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 1 '16 at 13:54
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.

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.