Convenience constructors and non-final setters

[Andreas Leitgeb]

This is a posting, asking about good programming style.
It's actually two separate questions on a similar setup.

Setup:

Let's say I want to create a class that encapsulates
a Pair of two differently typed things, each of which
may also be null.

class MyFoo {
private X x;
private Y y;

public X getX() { return x; }
public void setX(X vx) { /* verify validity of vx; */ x=vx; }
... ditto for y
}

The class is intended for customization, so neither itself,
nor its attribute-setters and getters shall be final.

Now I want to provide convenience constructors, as the class
is also intended to be conveniently useable "as is":

public MyFoo(X vx, Y vy) {
...
}

Question 1 (mostly about X):

It's said, that from a constructor I shall not call non-final methods,
but I also don't want to either bypass validity checks, nor duplicate
the validation code. Furthermore I'd like subclass constructors to
just pass on their constructors' arguments to the base-class and have
them verified by current class' possibly stronger validation checks
(through overridden setX()).

Oups, seemingly something has gone wrong here with my wishlist.
Is it just my wish for subclasses to be allowed to do "super(vx,vy);"
and have that care about polymorphic validation? Or is this an
acceptable exception to that rule, when documenting it clearly
that any override of setX must never store "this" externally, nor
assume completed construction of current class.
Or maybe the design fault is already some steps lower, than I'm
even aware of?

Question 2:

Multiple overloaded constructors taking either none,X,Y or both
(assuming non-polymorphic validation (i.e. final setX/Y) this
time, to factor out the issues from Q1).

public MyFoo() {}
public MyFoo(X vx) { setX(vx); }
public MyFoo(Y vy) { setY(vy); }
public MyFoo(X vx, Y vy) { setX(vx); setY(vy); }

or is it considered better to do it this way:

public MyFoo(X vx, Y vy) { setX(vx); setY(vy); }
public MyFoo(X vx) { this(vx,null); }
public MyFoo(Y vy) { this(null,vy); }
public MyFoo() { this(null,null); }

Assuming that null is documented to be a legal value, and
is being checked for right after entry into (now final) setX/Y.

I'm aware, that they aren't equivalent. If the differences really
counted in a given case at hand, there'd be no choice in the first
place. Perhaps others have experience with late demands likely
changing things towards forcing one or the other approach.

Finally:

Recommended styles are often about experience others have made. As such
they're not always entirely logically derivable from "I think therefore
I am"

 

[Joshua Cranmer]

It's said, that from a constructor I shall not call non-final methods,
but I also don't want to either bypass validity checks, nor duplicate
the validation code. Furthermore I'd like subclass constructors to
just pass on their constructors' arguments to the base-class and have
them verified by current class' possibly stronger validation checks
(through overridden setX()).

I think calling setX could be problematic if validation is complex. A
classic example of why calling non-final methods from a constructor can
be problematic is java.util.Random: its constructors call setSeed. On
the face of it, it seems "safe," since you're just setting the seed via
the constructor. Except that some RNGs require some per-instance
pre-initialization, which means that setSeed is called before it gets a
chance to initialize itself.

I don't feel comfortable giving a recommendation one way or another, so
I'll just leave you with that warning of what could happen and let you
plan your own course of action.

 

[Tom Anderson]

It's said, that from a constructor I shall not call non-final methods,
but I also don't want to either bypass validity checks, nor duplicate
the validation code. Furthermore I'd like subclass constructors to just
pass on their constructors' arguments to the base-class and have them
verified by current class' possibly stronger validation checks (through
overridden setX()).

Factor out the validation logic into separate methods. Then do:

// i'm ignoring Y for now
public class MyFoo {
private X x;
public MyFoo(X x) {
validate(x);
this.x = x;
}
public void setX(X x) {
validate(x);
this.x = x;
}
protected final void validate(X x) {
if (whatever) throw new IllegalArgumentException();
}
}

Alternatively, split the setter into two layers, a final inner layer which
can be used by the constructor, and an overridable outer layer:

public class MyFoo {
private X x;
public MyFoo(X x) {
_setX(x);
}
public void setX(X x) {
_setX(x);
}
protected final void _setX(X x) {
if (whatever) throw new IllegalArgumentException();
this.x = x;
}
}

The set of protected final methods surrounding the final variables
constitutes a kind of hard core to the class, within which you can
guarantee certain invariants no matter what, and the public non-final
methods are a soft outer layer which can be sculpted as necessary. Also,
if you decide to change the storage layout (eg the classic
cartesian-to-polar conversion of a Point), you only have to reimplement
the core methods - as long as the interface they expose to the other
methods stays the same, nothing else needs to change. That gives you a
degree of future-proofing.

Multiple overloaded constructors taking either none,X,Y or both
(assuming non-polymorphic validation (i.e. final setX/Y) this time, to
factor out the issues from Q1).

public MyFoo() {}
public MyFoo(X vx) { setX(vx); }
public MyFoo(Y vy) { setY(vy); }
public MyFoo(X vx, Y vy) { setX(vx); setY(vy); }

or is it considered better to do it this way:

public MyFoo(X vx, Y vy) { setX(vx); setY(vy); }
public MyFoo(X vx) { this(vx,null); }
public MyFoo(Y vy) { this(null,vy); }
public MyFoo() { this(null,null); }

I strongly prefer the latter. That way, i know that all constructor
invocations ultimately pass through a single point, and that gives me a
place to do anything which needs to always be done on construction (and
that isn't convenient to do in an instance initializer). That gives me
peace of mind, because i don't have to understand the details of N
constructors in order to convince myself that the code is right.

tom

 

[Lew]

Factor out the validation logic into separate methods. Then do:

// i'm ignoring Y for now
public class MyFoo {
private X x;
public MyFoo(X x) {
validate(x);
this.x = x;
}
public void setX(X x) {
validate(x);
this.x = x;
}
protected final void validate(X x) {
if (whatever) throw new IllegalArgumentException();
}
}

Alternatively, split the setter into two layers, a final inner layer
which can be used by the constructor, and an overridable outer layer:

public class MyFoo {
private X x;
public MyFoo(X x) {
_setX(x);
}
public void setX(X x) {
_setX(x);
}
protected final void _setX(X x) {
if (whatever) throw new IllegalArgumentException();
this.x = x;
}
}

The set of protected final methods surrounding the final variables
constitutes a kind of hard core to the class, within which you can

Most of the time if the superclass sports attributes, 'x' and 'y' in this
example, you want their access methods to be 'final'. If you're defining the
attribute in the superclass, it is usually a bad idea to allow subclasses any
control over those attributes. Subclasses are supposed to add attributes, not
reinvent those they inherit.

Usually.

See /Effective Java/, by Joshua Bloch.
<http://java.sun.com/docs/books/effective/index.html>
Item 17: "Design and document for inheritance or else prohibit it"
Item 16: "Favor composition over inheritance"

Object modeling and design are of the Dark Arts.

When you declare a member (always a method, I trust) 'protected' or 'public',
you incur the Responsibilities of the API Writer, which include sharply
curtailing the rights and privileges of future clients of that API.

 

[Andreas Leitgeb]

The cut from my original "bad wishlist" obviously is, that the baseclass
constructor only does baseclass's validation, and subclasses cannot just
hook into the validation itself (by overriding the setter), to strenghten
it but need to call their own stronger validation from both their constru-
ctor and their setX-overrides.

Which differs only in the detail, whether validation and actual
commiting to the new value is doen in one step. It has pros and cons.

If there was also a public isValidForX(X cand), then of course the former
wins, as there is another use for just validation without setting, otherwise
imho the latter (for not duplicating the assignment)

Most of the time if the superclass sports attributes, 'x' and 'y' in this
example, you want their access methods to be 'final'. If you're defining the
attribute in the superclass, it is usually a bad idea to allow subclasses any
control over those attributes.

An interesting restriction, whose value I do not *yet* see.
Since the fields are private, any subclass has no choice than use
baseclass's setter for setting them, so whatever validation the
subclass imposes on the attribute, it cannot be weaker than the
baseclass's.

I do, however, see the merits of making the getter final, to prevent
a subclass from bypassing the derived class entirely.

Anything I missed?

Subclasses are supposed to add attributes, not
reinvent those they inherit.

Imposing stronger rules on inherited attributes was in my list of
allowed (with respect to recommendations and OO-principles) things.

Should it not be?

I appreciate your opinions.

 

[Andreas Leitgeb]

I think calling setX could be problematic if validation is complex. A
classic example of why calling non-final methods from a constructor can
be problematic is java.util.Random: its constructors call setSeed. On
the face of it, it seems "safe," since you're just setting the seed via
the constructor.

It's insofar an interesting example, as it's not just an old sin, but I
still don't see how the problem could have been avoided initially.

Having to call setSeed() explicitly on user's side would have impacted
comfort severely (ok, I guess that argument doesn't count for OO-purists),
and determining the need to call it lazily only on each invocation of
next() would have been a performance issue. (another argument likely
not shared by OO-purists)

Are there even other ways out? (even if we could turn back time?)

Except that some RNGs require some per-instance pre-initialization,
which means that setSeed is called before it gets a chance to
initialize itself.

They'd have to override their setSeed() to eventually do the pre-init,
and then set some field to true (which I think would be definitely false
before the subclass-part is properly constructed.
I admit it's indeed far from nice, but deriving from Random surely
happens much rarelier than just using stock Random, so I understand
that convenience of Random itself was perhaps valued higher than
elegance for derivers.

I don't feel comfortable giving a recommendation one way or another,

I appreciate your referral to Random class as a very apt example
(good or bad) in this context.

 

[Joshua Cranmer]

Having to call setSeed() explicitly on user's side would have impacted
comfort severely (ok, I guess that argument doesn't count for OO-purists),
and determining the need to call it lazily only on each invocation of
next() would have been a performance issue. (another argument likely
not shared by OO-purists)

Are there even other ways out? (even if we could turn back time?)

I think the "cleanest" way out would have been to provide a protected
init() method which would have been called before setSeed.

I appreciate your referral to Random class as a very apt example
(good or bad) in this context.

It was what I immediately thought of .

 

[Andreas Leitgeb]

I think the "cleanest" way out would have been to provide a protected
init() method which would have been called before setSeed.

But that would still have broken the rule:
"no non-final (& non-private) method calls from constructor"

 

[Tom Anderson]

Most of the time if the superclass sports attributes, 'x' and 'y' in
this example, you want their access methods to be 'final'. If you're
defining the attribute in the superclass, it is usually a bad idea to
allow subclasses any control over those attributes. Subclasses are
supposed to add attributes, not reinvent those they inherit.

I couldn't possibly disagree more with any of what you've written there.

Usually.

Whether that's so or not, Andreas started this thread by saying:

The class is intended for customization, so neither itself, nor its
attribute-setters and getters shall be final.

So this is one of those unusual cases.

See /Effective Java/, by Joshua Bloch.
<http://java.sun.com/docs/books/effective/index.html>
Item 17: "Design and document for inheritance or else prohibit it"

I flat-out disagree with that one.

I do a lot of work with a large and complex web application framework,
where the kind of work we're trying to do at the moment requires extensive
customisation of all sorts of aspects. Far too many aspects for there to
have been explicit hooks to cover all of them, i think. Mostly, the
classes that comprise the framework are quite well-factored, with
separable tasks in separate methods, and thus we've been able to use
subclassing to do the customisation. Where we come across methods that are
final, or code that's badly factored, we instead have to spend an age
working around it. If the authors of the framework had employed a blanket
policy of non-inheritance, the project we're doing would be impossible.

If you need to prevent overriding or inheritance to enforce certain
invariants, then fine, that's the thing to do. If you want to provide
explicit extension hooks to guide and ease the job of subclassing, that's
great. But otherwise, leave the choice in the hands of the client
programmer.

How on earth can you know, at the point at which you're writing the class,
whether it's going to be a good idea or not to subclass it at some
arbitrary point in the future? Indeed, at ALL points in the future? That's
insane, wrong, monstrously arrogant, and a poster-child for how Big Design
Up Front goes wrong.

Item 16: "Favor composition over inheritance"

Object modeling and design are of the Dark Arts.

That i agree with.

When you declare a member (always a method, I trust) 'protected' or
'public', you incur the Responsibilities of the API Writer, which
include sharply curtailing the rights and privileges of future clients
of that API.

A somewhat Bushian take on matters . Based on my experiences as a
client, i prefer go give future clients the freedom to do what they like,
and let them take responsibility for their own actions.

tom

 

[Lew]

An interesting restriction, whose value I do not *yet* see.
Since the fields are private, any subclass has no choice than use
baseclass's setter for setting them, so whatever validation the
subclass imposes on the attribute, it cannot be weaker than the
baseclass's.

There are scenarios where that isn't true. For example, the derived class can
define its own 'x' and 'y' variables and ignore the parent class's altogether.

 

[Lew]

I flat-out disagree with that one.

Before or after reading the chapter? He provides a lot of reasons for the
principle.

Are you saying that if one does permit inheritance, then one should not design
and document for it?

 

[blue indigo]

There are scenarios where that isn't true. For example, the derived
class can define its own 'x' and 'y' variables and ignore the parent
class's altogether.

In many cases, if the derived class is going to reinvent much of the
functionality, the base class might be better off as an interface. If part
of the functionality is typically going to be reinvented and part isn't,
consider whether the base class is poorly factored. It may be better off
delegating the reinvented functionality to some inner policy/strategy
object. Alternatively, the base class might neither delegate nor implement
that functionality, leaving it as abstract methods.

But sometimes it does make sense for it to provide a default
implementation of some sort.

I think someone mentioned Point classes and implementations in this
newsgroup recently, and noted that proper encapsulation allowed
reimplementing with polar coordinates.

In this context, someone might be suggesting subclassing the Point to
ignore the parent's xy fields and use its own rd fields in polar
coordinates. In that case, it may be better to make Point and interface
and CartPoint and PolarPoint two separate implementations. In each place
where a Point is required, the best concrete type for the job can be
instantiated.

On the other hand, a ColoredPoint object or some other object with an x
and a y but also other things might be refactored to contain a Point
object (perhaps an interface rather than a concrete type) as above, and
delegate to that or simply expose a getter and possibly a setter for its
Point.

Or it could have abstract methods relating to its position, and
ColoredCartPoint and ColoredPolarPoint concrete subclasses that implement
the position functionality while inheriting the color functionality from
the base class.

However in that case having ColoredPoint implement cartesian coordinates
and ColoredPolarPoint subclass it and replace the position functionality
isn't 100% unreasonable, especially if someone else wrote ColoredPoint,
didn't provide anything else, and you for some reason desire
ColoredPolarPoint. The main downside is that the ColoredPolarPoint object
is a bit bigger than strictly necessary for its job, due to the unused
base class instance fields. If those are two doubles, that's sixteen extra
bytes per object, and it will add up if there are a lot of
ColoredPolarPoints at one time.

For reasons of efficiency and elegance, I'd ordinarily prefer not to have
disused instance state; I consider a desire to override parts of a class
and leave instance state unused to be a smell if I'm in control of the
base class, and typically refactor it to delegate or abstract the
associated functionality, or even make it an interface type.

I believe java.util.random has also been mentioned recently. It is a
classic case where it would have been better to provide an interface, or
perhaps a wholly abstract class with a factory method to supply the
default implementation. The model for the latter pattern would be
Calendar/GregorianCalendar, of course, and for the former List/ArrayList
or similarly. Making Random an interface could have avoided the entire
"constructor calls setSeed" mess, in particular.

 

[Joshua Cranmer]

But that would still have broken the rule:
"no non-final (& non-private) method calls from constructor"

I think, if suitably documented, it would be acceptable: the init()
would essentially become a replacement for the constructor. If a
constructor must call a non-final method call (esp. one expected to be
overridden), providing an initializer like that is better than providing
nothing at all, IMHO.

 

[blue indigo]

But that would still have broken the rule:
"no non-final (& non-private) method calls from constructor"

True. I think his point is that it might have mitigated the consequences,
IF subclass writers had their constructors only call super and do any
post-setSeed initialization, while doing all of their pre-setSeed
initialization in an overridden init() method. Then the superclass
constructor runs, init() runs, setSeed() runs, and the subclass
constructor runs.

It still stinks, though opinions may vary as to whether it stinks half as
bad or twice as bad. In my opinion they should have made Random abstract,
or even an interface. An abstract Random with a getInstance factory method
would have been cleaner than the mess we actually got.

 

[Andreas Leitgeb]

There are scenarios where that isn't true. For example, the derived class can
define its own 'x' and 'y' variables and ignore the parent class's altogether.

I did anticipate this one argument, so I wrote about making the getter
final. This way, while one can still replace the setter to do anything,
in order to impact what the getter returns, there'd be no way around
using baseclass' setter with all its validation checks.

Any arguments for making the setter final?

 

[Lew]

Any arguments for making the setter final?

You make the setter final when you want to forbid subclasses from
doing anything different with the attribute than what the parent class
does. You make the setter non-final when you want to allow the
subclass to do something different.

A non-final setter allows subclasses to do things with the attribute
argument before parent-class invariants hold. It's easy to think of
cases where this wouldn't matter, but in cases where it does it's nice
to have the option to prevent such things.

 

[Tom Anderson]

There are scenarios where that isn't true. For example, the derived
class can define its own 'x' and 'y' variables and ignore the parent
class's altogether.

True, but those x and y variables do not replace the superclass's x and y,
they're totally different variables. Any methods which use the superclass
x and y will continue to use validated values.

Unless those methods are also overriden, of course!

tom

 

[Tom Anderson]

Before or after reading the chapter?

I'm afraid i don't have a copy of the book, so i havem't read it at all

He provides a lot of reasons for the principle.

I'm sure he does, and if i had a copy of the book, i might be persuaded by
them. But all i have to go on is the conclusion, which i strongly disagree
with.

Are you saying that if one does permit inheritance, then one should not
design and document for it?

Not at all. I believe i made my opinion quite clear in my previous post;
if on re-reading that, it's not clear to you, i'd be happy to explain.

tom

 

[Lew]

Not at all. I believe i made my opinion quite clear in my previous post;
if on re-reading that, it's not clear to you, i'd be happy to explain.

Yes, please. The rule is either design and document for inheritance,
or prohibit it. If you "flat-out disagree" with that, then you must
think there are times when one does not prohibit inheritance, nor
designs and documents for it. I am puzzled at how you can support
such a notion. Shouldn't you do one or the other, always?

 

[Lew]

I'm afraid i don't have a copy of the book, so i havem't read it at all


I'm sure he does, and if i had a copy of the book, i might be persuaded by
them. But all i have to go on is the conclusion, which i strongly disagree
with.

You should maybe read the arguments before you make up your mind.
Deciding in advance of evidence is the definition of prejudice.