All those constructors...

[Paul Chapman]

I'm a Java novice, but an experienced Smalltalk and C++ programmer.

I have this fairly large class heirarchy, with all but the leaves abstract.
One of the base class's fields is to be initialized with a value provided to
a static factory method which selects the right subclass, *except* in one
leaf case where it is replaced with something else.

I find I have to have a constructor in every class which simply calls super
with the same argument all the way up to the top.

It would frankly be a lot less code to have a separate setField() method to
set that field, which is defined in the base class and overridden in that
one case.

I am torn between doing it the "right" way, with all those extra and quite
pontless super-calling constructors, and the "wrong" way which cuts them
out.

I'm someone who generally believes (up to a point) that *short* code is more
easily understood and maintained than *long* code. Should I carry this
belief into Java in this instance?

Cheers, Paul

 

[John C. Bollinger]

I'm a Java novice, but an experienced Smalltalk and C++ programmer.

I have this fairly large class heirarchy, with all but the leaves abstract.
One of the base class's fields is to be initialized with a value provided to
a static factory method which selects the right subclass, *except* in one
leaf case where it is replaced with something else.

I tend to get suspicious when I hear about deep class hierarchies, but
it may be warranted in your case, and anyway that's a topic for a
different thread.

I find I have to have a constructor in every class which simply calls super
with the same argument all the way up to the top.

It would frankly be a lot less code to have a separate setField() method to
set that field, which is defined in the base class and overridden in that
one case.

Why does the method need to be overridden at all? If the field is
supposed to be mutable, then its mutator is properly defined on the base
class, and it oughtn't to need to be overridden. If the base class
constructor is going to invoke it then make the method final. If you
want to provide some measure of protection then make the mutator
"protected" or default access.

Alternatively, since you're using a factory method to create instances,
you don't necessarily need any extra method at all: create the object,
and in the one case have the factory method (defined on the base class,
I'm assuming) set the field directly.

I am torn between doing it the "right" way, with all those extra and quite
pontless super-calling constructors, and the "wrong" way which cuts them
out.

You have a bunch of super-calling constructors provided for you anyway,
but, true, this would be an additional set of them. I don't think it's
necessary.

I'm someone who generally believes (up to a point) that *short* code is more
easily understood and maintained than *long* code. Should I carry this
belief into Java in this instance?

I don't see why not.

As long as you're bringing up understanding and maintaining code,
though, I'll point out that it sounds like part of the problem is the
depth of your class hierarchy. _I'm_ someone who is coming to believe
more and more strongly that shallow class hierarchies that make good use
of composition are more easily understood and maintained than deep
hierarchies that rely heavily on inheritance and method overriding. But
oops, there I go again.

 

[Paul Chapman]

John,

I tend to get suspicious when I hear about deep class hierarchies, but
it may be warranted in your case, and anyway that's a topic for a
different thread.

Not that deep so far:

Mapper
IndexedMapper
ByteMapper
DictMapper
RecordMapper
RecordClassMapper

And in fact, only Mapper is abstract at the moment.

Alternatively, since you're using a factory method to create instances,
you don't necessarily need any extra method at all: create the object,
and in the one case have the factory method (defined on the base class,
I'm assuming) set the field directly.

That's what I decided to do:

public static Mapper New(Attr owner, byte[] bytes, int size)
{
Mapper result =
owner.isRecordClass() ? (Mapper) new RecordClassMapper() :
owner.isRecord() ? (Mapper) new RecordMapper() :
!owner.isIndexed() ? (Mapper) new DictMapper() :
owner.isBytes() ? (Mapper) new ByteMapper() :
(Mapper) new IndexedMapper() ;
result.initialize(owner, bytes, size);
return result;
}

initialize is now overridden in ByteMapper to do something slightly
different with bytes. As it happens, only one of the three fields
initialize fills )owner) is in theory final. I can live without that bit of
protection (and self-documentation).

You have a bunch of super-calling constructors provided for you anyway,
but, true, this would be an additional set of them. I don't think it's
necessary.

Me neither.

As long as you're bringing up understanding and maintaining code,
though, I'll point out that it sounds like part of the problem is the
depth of your class hierarchy. _I'm_ someone who is coming to believe
more and more strongly that shallow class hierarchies that make good use
of composition are more easily understood and maintained than deep
hierarchies that rely heavily on inheritance and method overriding. But
oops, there I go again.

By "composition" do you mean something like breaking the heirarchy "half way
down" and putting the bottom half in a field of the base class? There is in
fact another (very shallow) heirachy in one of Mapper's fields. I've been
moving the cut-off point up and down for the past few days, trying to find
where it maskes most sense.

Thanks for the pointers. (Can you say "pointers" in a Java ng?)

Cheers, Paul

 

[Michael Klaus]

I have this fairly large class heirarchy, with all but the leaves
abstract. One of the base class's fields is to be initialized with a value
provided to a static factory method which selects the right subclass,
*except* in one leaf case where it is replaced with something else.

I had a quite similar case and solved it (in my eyes fairly clean) by:
- providing a protected default constructor in the node class
- creating package visible setXXX() for the init parameters
- creating a public init() method which subclasses use for their constructor
code
- putting the factory into the same package with the node classes

The drawback is, you can not programatically ensure implementors use the
init() method instead of the constructor. This has to be done via
documentation. As I said: fairly clean ;o)

 

[Andrew McDonagh]

John,

I'm the same as John WRT class hierarchies, tending to favour
composition & delegation over inheritance.

Not that deep so far:

Mapper
IndexedMapper
ByteMapper
DictMapper
RecordMapper
RecordClassMapper

And in fact, only Mapper is abstract at the moment.

True, but my own nose is saying that its getting close..

Alternatively, since you're using a factory method to create instances,
you don't necessarily need any extra method at all: create the object,
and in the one case have the factory method (defined on the base class,
I'm assuming) set the field directly.

That's what I decided to do:

public static Mapper New(Attr owner, byte[] bytes, int size)
{
Mapper result =
owner.isRecordClass() ? (Mapper) new RecordClassMapper() :
owner.isRecord() ? (Mapper) new RecordMapper() :
!owner.isIndexed() ? (Mapper) new DictMapper() :
owner.isBytes() ? (Mapper) new ByteMapper() :
(Mapper) new IndexedMapper() ;
result.initialize(owner, bytes, size);
return result;
}

Another alternative would be to move this factory method into the Attr
class and make it an instance method instead of a static class method.
Then it can decide which actual concrete Mapper class to instantiate
with whatever correct initialisation is required.

The Attr class would need to be a class hierarchy with the subclasses:
RecordClassAttr, RecordAttr, DictAttr, ByteAttr and IndexedAttr.

Then you'd be able do...

Mapper mapper = attr.createMapper();

class RecordClassAttr extends Attr {
Mapper createMapper() {
return new RecordClassMapper();
}
}

....

class DictAttr extends Attr {
Mapper createMapper() {
return new DictMapper();
}
}

snipped

 

[Paul Chapman]

Andrew,

True, but my own nose is saying that its getting close..

Aye, perhaps. ByteMapper and RecordCLassMapper are pretty small, though.
Some of the specialization of ByteMapper is delegated to a field of Mapper,
which in this one case has a different class. I could probably move all of
it there.

public static Mapper New(Attr owner, byte[] bytes, int size)
{
Mapper result =
owner.isRecordClass() ? (Mapper) new RecordClassMapper() :
owner.isRecord() ? (Mapper) new RecordMapper() :
!owner.isIndexed() ? (Mapper) new DictMapper() :
owner.isBytes() ? (Mapper) new ByteMapper() :
(Mapper) new IndexedMapper() ;
result.initialize(owner, bytes, size);
return result;
}

Another alternative would be to move this factory method into the Attr
class and make it an instance method instead of a static class method.
Then it can decide which actual concrete Mapper class to instantiate
with whatever correct initialisation is required.

A previous sketch did it that way. In the end, since most of the package
knows about the Attr.is*() methods, but only Attr knows about Mapper, it
seemed to make more sense where I now have it.

The Attr class would need to be a class hierarchy with the subclasses:
RecordClassAttr, RecordAttr, DictAttr, ByteAttr and IndexedAttr.

I've thought about that, too. But such subclasses wouldn't really be what
Attr itself (or rather, AttrInterface; I've decided to help document the
public methods in important classes by defining their own interfaces, which
also adds another level of compile-time checks) is mainly about.

I ask these questions mainly because I believe when one moves into a new
language, one should try one's best to honour the conventions of that
language's community. After all, Java programmers end up maintaining Java
programs, and they'd hate to come across a Java program written in the
Smalltalk style.

Cheers, Paul

 

[John C. Bollinger]

By "composition" do you mean something like breaking the heirarchy "half way
down" and putting the bottom half in a field of the base class? There is in
fact another (very shallow) heirachy in one of Mapper's fields. I've been
moving the cut-off point up and down for the past few days, trying to find
where it maskes most sense.

When you need customizable behavior there are two non-exclusive approaches:

(1) Establish a class hierarchy in which methods are overridden as
necessary to arrive at a collection of classes, each of which exhibits
the behavior required of some subset of the objects that will be used.

(2) Create a class whose methods delegate to one or more objects stored
in fields of the class, and adjust behavior of class instances by the
selection of the delegates (class choice and/or instance configuration).


Appropriate use of the latter approach can lead to applications with
fewer and simpler classes. It can also be a solution for code reuse in
situations where otherwise classes in different branches of your
hierarchy would need to separately implement a common behavior.

I recently took a class hierarchy with fourteen classes, three levels
deep, and decomposed it into one main class, one two-class hierarchy,
and one four-class, two-level hierarchy with the top level abstract.
That's fourteen classes down to seven, and most of those seven quite
simple. The result was much easier to understand, and also easier to
test (JUnit).


John Bollinger
(e-mail address removed)

 

[Paul Chapman]

John,

I recently took a class hierarchy with fourteen classes, three levels
deep, and decomposed it into one main class, one two-class hierarchy,
and one four-class, two-level hierarchy with the top level abstract.
That's fourteen classes down to seven, and most of those seven quite
simple. The result was much easier to understand, and also easier to
test (JUnit).

I would expect that this would increase the number of conditionals.

My app is an ODBMS. One of the (currently) four methods overridden in
ByteMapper (extends IndexedMapper) is:

public void put(Handle key, Handle value) throws ODBException
{
if (!value.isByte()) throw new ODBException();
super.put(key, value);
}

I could implement this behaviour in IndexedMapper.put() by writing:

if (owner.isBytes() && !value.isByte()) throw new ODBException();

isBytes() is a very cheap operation. Is this an example where you would
eliminate class ByteMapper, using "if (owner.isBytes()) ..." where
appropriate to differentiate behaviour?

Cheers, Paul

 

[Paul Chapman]

John,

This is a slightly more interesting example. Here is the whole of
RecordClassMapper, the other class deep in the heirarchy:

class RecordClassMapper extends RecordMapper
{
private Handle[] instanceKeys;

public Handle[] instanceKeys()
{
if (instanceKeys == null)
{
Obj keysObject = owner.cache.get(owner.instanceKeysAttr());
instanceKeys = new Handle[keysObject.size()];

try
{
for (int i = 0; i < instanceKeys.length; i++)
instanceKeys = keysObject.get(Handle.ForInteger(i));
}
catch (ODBException e) {throw new ODBRuntimeException();}
}

return instanceKeys;
}
}

It represents the class object of "record-type" instances in the ODBMS, ie
objects with fixed, named fields. It serves merely to cache the field names
(keys) of its instances.

instanceKeys() can be guaranteed only to be called for class objects, so
could equally well reside in RecordMapper. But instanceKeys[] is particular
to class objects, and so would sometimes be a redundant field in
RecordMapper.

Would you nevertheless eliminate RecordClassMapper? I'm not sure I like
part-time fields.

Cheers, Paul

 

[John C. Bollinger]

I would expect that this would increase the number of conditionals.

In my case, no, it actually reduced the number of conditionals considerably.

My app is an ODBMS. One of the (currently) four methods overridden in
ByteMapper (extends IndexedMapper) is:

public void put(Handle key, Handle value) throws ODBException
{
if (!value.isByte()) throw new ODBException();
super.put(key, value);
}

I could implement this behaviour in IndexedMapper.put() by writing:

if (owner.isBytes() && !value.isByte()) throw new ODBException();

isBytes() is a very cheap operation. Is this an example where you would
eliminate class ByteMapper, using "if (owner.isBytes()) ..." where
appropriate to differentiate behaviour?

One composition solution for that particular scenario would be to create
a TypeVerifier interface with a suitable collection of implementations,
one of which might be the trivial one, and another of which might be a
ByteVerifier. Each IndexedMapper (or possibly each Mapper) has a
TypeVerifier of an appropriate kind, and the code in IndexedMapper might
be something like

public void put(Handle key, Handle value) throws ODBException {
verifier.verify(value); // throws ODBException if not OK

// continue with the putting ...

}

That by itself is probably not enough to persuade anyone, although it
does have the advantage of encapsulating the type checking logic (in
case you also need to use it again in get() and delete(), for instance).

When you look to decompose a class hierarchy, the relevant questions are
"What differentiates these classes?" and then "Can I represent that in
its own class?".


John Bollinger
(e-mail address removed)

 

[Paul Chapman]

John,

One composition solution for that particular scenario would be to create
a TypeVerifier interface with a suitable collection of implementations,
one of which might be the trivial one, and another of which might be a
ByteVerifier.

In this case, I think you'd agree that would be overkill. But I take the
general point.

I note you picked, probably without thinking about it, a solution involving
an interface and two (otherwise) unrelated classes, rather than either an
abstract superclass and two subclasses, or two classes, one extending the
other. The lesson for me here is that I should probably think about
interfaces more, since they don't exist in Smalltalk (where they can be
badly approximated by wrappers) or C++ (where they can be badly approximated
by multiple inheritance).

OTOH, Smalltalk has lexical closures (which can be approximated by nested
and anonymous classes in Java, and function/method pointers in C++ which I
would never use). In Smalltalk, these can help reduce the number of tiny
classes quite neatly.

Cheers, Paul

 

[John C. Bollinger]

This is a slightly more interesting example. Here is the whole of
RecordClassMapper, the other class deep in the heirarchy:

class RecordClassMapper extends RecordMapper
{
private Handle[] instanceKeys;

public Handle[] instanceKeys()
{
if (instanceKeys == null)
{
Obj keysObject = owner.cache.get(owner.instanceKeysAttr());
instanceKeys = new Handle[keysObject.size()];

try
{
for (int i = 0; i < instanceKeys.length; i++)
instanceKeys = keysObject.get(Handle.ForInteger(i));
}
catch (ODBException e) {throw new ODBRuntimeException();}
}

return instanceKeys;
}
}

It represents the class object of "record-type" instances in the ODBMS, ie
objects with fixed, named fields. It serves merely to cache the field names
(keys) of its instances.

instanceKeys() can be guaranteed only to be called for class objects, so
could equally well reside in RecordMapper. But instanceKeys[] is particular
to class objects, and so would sometimes be a redundant field in
RecordMapper.

Would you nevertheless eliminate RecordClassMapper? I'm not sure I like
part-time fields.

It is very difficult to make these kinds of decisions looking only at
the leaf classes instead of the whole hierarchy, and without really
knowing much about the envisioned use cases for each class. I certainly
would _consider_ eliminating this class, and that does not necessarily
imply putting the instanceKeys field on RecordMapper. If I wanted to
replace this inheritance situation with composition, then I would
probably put the instanceKeys field into whatever class I created to
delegate this behavior to.

 

[Paul Chapman]

John,

... If I wanted to
replace this inheritance situation with composition, then I would
probably put the instanceKeys field into whatever class I created to
delegate this behavior to.

Well, in this case the Mapper heirarchy is already functioning as a delegate
for the Attr class.

The Attr class would need to be a class hierarchy with the subclasses:
RecordClassAttr, RecordAttr, DictAttr, ByteAttr and IndexedAttr.

It was to avoid this very thing that I added the Mapper heirarchy.
Furthermore, the Mapper class already delegates some behaviour to another
small heirarchy of classes which manages access to the raw byte-array which
contains the actual data. There does not seem to me to be a further cut-off
point within the Mapper heirarchy for yet another level of delegation.

Thanks for all your help, anyway.

Cheers, Paul