Please comment on this class hierarchy design

[Avi Abrami]

The following code is a minimalized version of a class
hierarchy, designed by one of my co-workers, that is
part of our java application software. He insists that
it is a good design, but I, and several other co-workers,
think it is a bad design. What do you think?
(Note: I'm not asking for alternatives, just whether you
think it's good -- or bad -- and why.)

public abstract class A {
protected static final int CHILD_TYPE_B = 0;
protected static final int CHILD_TYPE_C = 1;

protected BMember bMember;
protected CMember cMember;
protected Collection bList;
protected Collection cList;

private int childType;

// Constructor
public A(int type) {
childType = type;
switch (childType) {
case CHILD_TYPE_B:
bMember = new BMember();
bList = new ArrayList();
break;
case CHILD_TYPE_C:
cMember = new CMember();
cList = new ArrayList();
break;
}
}
}

public class B extends A {

//Constructor
public B() {
super(CHILD_TYPE_B);
}
}

public class C extends A {

// constructor
public C() {
super(CHILD_TYPE_C);
}
}

public class Member {
}

public class BMember extends Member {
}

public class CMember extends Member {
}

Thanks (in advance),
Avi.

 

[Marco Schmidt]

Avi Abrami:

The following code is a minimalized version of a class
hierarchy, designed by one of my co-workers, that is
part of our java application software. He insists that
it is a good design, but I, and several other co-workers,
think it is a bad design. What do you think?
(Note: I'm not asking for alternatives, just whether you
think it's good -- or bad -- and why.)

Instead of really making use of inheritance, the base class A already
knows about its extending classes. Every new extension D, E etc. would
lead to a modification of A. This smells like very bad design. Imagine
a new extension of java.util.AbstractList that would have to modify
AbstractList itself.

The more natural OOP approach would be:

public abstract class A {
protected Collection list = new ArrayList();
}

public class B extends A {
protected BMember bMember = new BMember();
}

public class C extends A {
protected CMember cMember = new CMember();
}

or

public abstract class A {
protected Collection list = new ArrayList();
protected Member member;
}

public class B extends A {
public B() {
member = new BMember();
}
}

public class C extends A {
public C() {
member = new CMember();
}
}

However, a "real" evaluation of the code is impossible. I'd have to
know more about what A, B, C, Member, BMember and CMember do. How do
they interact? That's why I'm unclear whether that (B|C)Member field
should be in A or in its heirs B and C.

Maybe BMember and CMember add new methods that must be called in B and
C. A lot of typecasts would be necessary to get a BMember and CMember
back from a Member field. That's when I'd prefer my first approach.

On the other hand, if BMember and CMember only implement or modify
methods from Member, the second approach is probably better.

Regards,
Marco

 

[xarax]

The following code is a minimalized version of a class
hierarchy, designed by one of my co-workers, that is
part of our java application software. He insists that
it is a good design, but I, and several other co-workers,
think it is a bad design. What do you think?
(Note: I'm not asking for alternatives, just whether you
think it's good -- or bad -- and why.)

/snip/

In addition to the other comments, you should also
be wary of any occurance of a switch(){} block that
is used to vary the processing depending on the
type of an object. Such type-dependent processing
should be delegated to the child class, usually through
abstract methods in the parent class.

However, some purists are strongly averse to calling
abstract methods within a constructor, transfering
control to the uninitialized child class. With careful
design and implementation, it's not all that bad, and
certainly more object-oriented than using switch blocks.

Having said all of that, I do have some package-private
classes that are very similar to what you posted (no
switch block) that are programmatically generated. The
use of type codes to enumerate the various subclasses
is a safety check to be sure that I *do not* add more
subclasses. The set of subclasses is fixed; the type
codes are an easy way to verify that subclasses are
not added incompatibly. (Remember these classes are
programmatically generated, rather than handwritten.)

/xarax

 

[newton klea]

it is a bad design. he doesn't have an understanding of oo just yet.
another thing, for java now aday i rarely see any one use abstract
class. use interface instead for it is better.

 

[Jesper Nordenberg]

The following code is a minimalized version of a class
hierarchy, designed by one of my co-workers, that is
part of our java application software. He insists that
it is a good design, but I, and several other co-workers,
think it is a bad design. What do you think?

It's bad. I'll point out why below.

public abstract class A {
protected static final int CHILD_TYPE_B = 0;
protected static final int CHILD_TYPE_C = 1;

Looks like an enum. Don't use int's, use a type safe enumeration
pattern:

http://members.tripod.com/rwald/java/articles/TypeSafeEnumeration.html

In this example there is no need for an enumeration though.

protected BMember bMember;
protected CMember cMember;
protected Collection bList;
protected Collection cList;

Don't use protected fields. Use private fields and getters/setters
(which can be protected). Subclasses can mess up these fields if
you're not careful.

private int childType;

// Constructor
public A(int type) {
childType = type;
switch (childType) {

Switch statements always have a bad smell. Almost all switch
statements should be replaced with inheritance and polymorphism.

case CHILD_TYPE_B:
bMember = new BMember();
bList = new ArrayList();
break;
case CHILD_TYPE_C:
cMember = new CMember();
cList = new ArrayList();
break;

Looks like either bMember and bList, or cMember and cList are used
which means you have unused fields which wastes memory and makes the
class confusing. To me it looks like bMember and bList should go into
the B class, and cMember and cList should go into the C class. I don't
see what bMember and cMember is used for though. Maybe they can be
removed.

public class Member {
}

public class BMember extends Member {
}

public class CMember extends Member {
}

Empty classes are always a bad smell ;-)

/Jesper Nordenberg

 

[Jason]

it is a bad design. he doesn't have an understanding of oo just yet.
another thing, for java now aday i rarely see any one use abstract
class. use interface instead for it is better.

I've been known to use both, but not interchangeably. An abstract
class is very useful for things that you don't want to instantiate,
for example a factory class that has a single method who's job it is
to spit up an instance of several related classes based on some
criterion.

I would be careful of the blanket statement that an interface is
"better" ... It is certainly better for some things and in some
implementations, most quite probably, but there are cases and
situations where an abstract class is not only viable, it's also an
exceptional resolution.

Just my 2 krupplenicks on the subject.

 

[rkm]

Is a krupplenick a coin or is that just a common expression
in your country? What country is that?

rkm