Looking for a way to avoid use of instanceOf

[François Grondin]

Hi everyone

I have the following problem. I'm developing a factory method that creates
objects from a list. This list contains objects that implements my interface
IRegulatedNME. Three classes implements this interface : Gate, Pump and
Weir. My factory method looks like this (I hope this code is clear enough) :

private List<AbstractConstraint> create(List<IRegulatedNME> aRegNMEList)
{
List<AbstractConstraint> ret = new ArrayList<AbstractConstraint>();

for (IRegulatedNME regNME : aRegNMEList)
{
if (regNME.isGloballyCtrl())
{
ret.add(new GloballyCtrlConstraint(regNME);
}
else if (regNME.isLocallyCtrl())
{
ret.add(new LocallyCtrlConstraint(regNME);
}
else if (regNME.isUnctrl())
{
if (regNME instanceof Gate)
{
ret.add(new UnctrlGateConstraint((Gate) regNME);
}
else if (regNME instanceof Pump)
{
ret.add(new UnctrlPumpConstraint((Pump) regNME);
}
else // if (regNME instanceof Weir)
{
ret.add(new UnctrlWeirConstraint((Weir) regNME);
}
}
}

return ret;
}

As you could see, the cases "regNME.isGloballyCtrl()" or
"regNME.isLocallyCtrl()" apply to the abstract object. When
"regNME.isUnctrl()", then the constraints to be added depends on the type of
regNME (Gate, Pump or Weir). The list "aRegNMEList" may contain gates, pumps
or weirs at the same time. Is there a way to avoid the use of "instanceof"
in that situation? Could the generics be useful to solve this problem? Is
there a way to filter my list to extract only one type at a time (without
using "instanceof" of course)?

I googled "replace instanceof java" and "filter collection java" but was not
satisfied with the answers.

Thanks in advance for your replies.

François

 

[Stefan Rybacki]

Hi everyone

I have the following problem. I'm developing a factory method that creates
objects from a list. This list contains objects that implements my interface
IRegulatedNME. Three classes implements this interface : Gate, Pump and
Weir. My factory method looks like this (I hope this code is clear enough) :

private List<AbstractConstraint> create(List<IRegulatedNME> aRegNMEList)
{
List<AbstractConstraint> ret = new ArrayList<AbstractConstraint>();

for (IRegulatedNME regNME : aRegNMEList)
{
if (regNME.isGloballyCtrl())
{
ret.add(new GloballyCtrlConstraint(regNME);
}
else if (regNME.isLocallyCtrl())
{
ret.add(new LocallyCtrlConstraint(regNME);
}
else if (regNME.isUnctrl())
{
if (regNME instanceof Gate)
{
ret.add(new UnctrlGateConstraint((Gate) regNME);
}
else if (regNME instanceof Pump)
{
ret.add(new UnctrlPumpConstraint((Pump) regNME);
}
else // if (regNME instanceof Weir)
{
ret.add(new UnctrlWeirConstraint((Weir) regNME);
}
}
}

return ret;
}

As you could see, the cases "regNME.isGloballyCtrl()" or
"regNME.isLocallyCtrl()" apply to the abstract object. When
"regNME.isUnctrl()", then the constraints to be added depends on the type of
regNME (Gate, Pump or Weir). The list "aRegNMEList" may contain gates, pumps
or weirs at the same time. Is there a way to avoid the use of "instanceof"
in that situation? Could the generics be useful to solve this problem? Is
there a way to filter my list to extract only one type at a time (without
using "instanceof" of course)?

I googled "replace instanceof java" and "filter collection java" but was not
satisfied with the answers.

maybe the IRegulatedNME interface can implement a method called
AbstractConstraint getConstraint();
This way you can avoid instanceof and furthermore you can support any
AbstractConstraint without modifying/adding new instanceof clauses for each new
available constraint.
But I don't know whether it makes sense to let the regNME provide its constraints.

Regards
Stefan

 

[François Grondin]

maybe the IRegulatedNME interface can implement a method called
AbstractConstraint getConstraint();
This way you can avoid instanceof and furthermore you can support any
AbstractConstraint without modifying/adding new instanceof clauses for
each new
available constraint.
But I don't know whether it makes sense to let the regNME provide its
constraints.

Regards
Stefan

Hi Stefan

Thanks for your reply. Effectively, I don't like the idea of adding a method
getConstraint() in IRegulatedNME.

I forgot to mention that I may construct more than one constraint at a time.
My example should be :

private List<AbstractConstraint> create(List<IRegulatedNME> aRegNMEList)
{
List<AbstractConstraint> ret = new ArrayList<AbstractConstraint>();

for (IRegulatedNME regNME : aRegNMEList)
{
if (regNME.isGloballyCtrl())
{
ret.add(new GloballyCtrlConstraint(regNME);
ret.add(new AnotherGloballyCtrlConstraint(regNME);
}
else if (regNME.isLocallyCtrl())
{
ret.add(new LocallyCtrlConstraint(regNME);
}
else if (regNME.isUnctrl())
{
ret.add(new AnotherConstraint(regNME);

if (regNME instanceof Gate)
{
ret.add(new UnctrlGateConstraint((Gate) regNME);
ret.add(new AnotherUnctrlGateConstraint((Gate) regNME);
}
else if (regNME instanceof Pump)
{
ret.add(new UnctrlPumpConstraint((Pump) regNME);
}
else // if (regNME instanceof Weir)
{
ret.add(new UnctrlWeirConstraint((Weir) regNME);
}
}
}

return ret;
}

This is closer to what I really need.

Regards

Francois

 

[Stefan Ram]

ret.add(new UnctrlGateConstraint((Gate) regNME);

This is indecipherable because the parentheses do not match.

 

[François Grondin]

This is indecipherable because the parentheses do not match.

OK. Here is a more complete and corrected version of the code.

private List<AbstractConstraint> create(List<IRegulatedNME> aRegNMEList)
{
List<AbstractConstraint> ret = new ArrayList<AbstractConstraint>();

for (IRegulatedNME regNME : aRegNMEList)
{
if (regNME.isGloballyCtrl())
{
ret.add(new GloballyCtrlConstraint(regNME));
ret.add(new AnotherGloballyCtrlConstraint(regNME));
}
else if (regNME.isLocallyCtrl())
{
ret.add(new LocallyCtrlConstraint(regNME));
}
else if (regNME.isUnctrl())
{
ret.add(new AnotherConstraint(regNME));

if (regNME instanceof Gate)
{
ret.add(new UnctrlGateConstraint((Gate) regNME));
ret.add(new AnotherUnctrlGateConstraint((Gate) regNME));
}
else if (regNME instanceof Pump)
{
ret.add(new UnctrlPumpConstraint((Pump) regNME));
}
else // if (regNME instanceof Weir)
{
ret.add(new UnctrlWeirConstraint((Weir) regNME));
}
}
}

return ret;
}

I hope this is clearer this time. Sorry for the inconvenience.

François

 

[Andreas Leitgeb]

Thanks for your reply. Effectively, I don't like the idea of adding a method
getConstraint() in IRegulatedNME.

I forgot to mention that I may construct more than one constraint at a time.
My example should be :

how about getListOfConstraints() then?

ret.add(new GloballyCtrlConstraint(regNME);
ret.add(new AnotherGloballyCtrlConstraint(regNME);

for (BaseConstraint bc : regNME.getListOfConstraints() ) ret.add(bc);

If you cannot add it to regNME, then there is hardly any way to avoid
those various instanceof's.

Oh, there is another one: add an enumeration with all the regNME types
and do a switch on regNME.myType()

PS: please take the effort to trim the quotes.

 

[Stefan Ram]

if (regNME instanceof Gate)
{
ret.add(new UnctrlGateConstraint((Gate) regNME));
ret.add(new AnotherUnctrlGateConstraint((Gate) regNME));
}
else if (regNME instanceof Pump)
{
ret.add(new UnctrlPumpConstraint((Pump) regNME));
}
else // if (regNME instanceof Weir)
{
ret.add(new UnctrlWeirConstraint((Weir) regNME));
}

regNME.addTo( ret );

, where the implementation of »addTo« differs
for »Gate«, »Pump« and »Weir« as given above.

»This is the heart of why ObjectOrientedProgramming is better.«

http://c2.com/cgi/wiki?ReplaceConditionalWithPolymorphism

 

[Tom Anderson]

This is indecipherable because the parentheses do not match.

No, it's incorrect because the parentheses do not match, but it's trivial
to decipher. There's nowhere you could take an open paren out and have it
be correct, which means there's a close paren missing, and there's only
one place it would be syntactically permitted. It's also obvious what this
line is doing semantically, and thus where the missing paren belongs.

I have no problem with pointing out that people's code is wrong, but
please, let's keep a sense of perspective. We don't do anyone any favours
by getting preoccupied with minutiae like this.

tom

 

[Tom Anderson]

One minor tiny niggle: in java, it's not conventional to stick an 'I' on
the front of an interface name. RegulatedNME is fine. And if you can find
a proper word that can be used instead of NME, that would be beautiful!

how about getListOfConstraints() then?

Yup. Personally, i'd call it getConstraints(), and have it return a Set,
unless there's an ordering on constraints. Or it could return a
Collection.

Francois [1], you might well not like this solution, but it's definitely
the Official Orthodox Object-Oriented solution. Not that i'm saying it's
the right solution - you might see constraints as being a separate concern
from whatever it is that RegulatedNMEs are mostly about.

for (BaseConstraint bc : regNME.getListOfConstraints() ) ret.add(bc);

If you cannot add it to regNME, then there is hardly any way to avoid
those various instanceof's.

Oh, there is another one: add an enumeration with all the regNME types
and do a switch on regNME.myType()

That's not a bad idea.

While we're on the subject, i'd also use an enumeration for the control
state; rather than those three boolean isXXXCtrl(), methods, define:

enum Control {
GLOBAL,
LOCAL,
UNCONTROLLED
}

And have a method:

public Control getControl() ;

Then instead of the if-else-if cascade, do:

switch (regNME.getControl()) {
case GLOBAL:
// etc
case LOCAL:
// etc
case UNCONTROLLED:
// etc
}

Anyway, back to the question of replacing the instanceof. There is another
way: bounce dispatch. This is an arcane technique of the OO masters, which
i will now share with you. It goes by many names - i don't think it has a
standard one. Most people will know it from the Visitor pattern, but it's
not the same as Visitor - Visitor is this plus the Internal Iterator
pattern.

The key is an interface like this:

interface RegulatedNMEHandler {
public void handle(Gate gate) ;
public void handle(Pump pump) ;
public void handle(Weir weir) ;
}

You then add a method to RegulatedNME:

interface RegulatedNME {
public void accept(RegulatedNMEHandler hdlr) ;
}

With implementations that look like this:

class Gate implements RegulatedNME {
public void accept(RegulatedNMEHandler hdlr) {
hdlr.handle(this) ;
}
}

You now write your method like this:

private List<AbstractConstraint> create(List<RegulatedNME> aRegNMEList)
{
final List<AbstractConstraint> ret = new ArrayList<AbstractConstraint>();
for (RegulatedNME regNME : aRegNMEList)
{
if (regNME.isGloballyCtrl())
{
ret.add(new GloballyCtrlConstraint(regNME));
ret.add(new AnotherGloballyCtrlConstraint(regNME));
}
else if (regNME.isLocallyCtrl())
{
ret.add(new LocallyCtrlConstraint(regNME));
}
else if (regNME.isUnctrl())
{
ret.add(new AnotherConstraint(regNME));
regNME.accept(new RegulatedNMEHandler() {
public void handle(Gate gate) {
ret.add(new UnctrlGateConstraint(gate);
ret.add(new AnotherUnctrlGateConstraint(gate);
}
public void handle(Pump pump) {
ret.add(new UnctrlPumpConstraint(pump);
}
public void handle(Weir weir) {
ret.add(new UnctrlWeirConstraint(weir);
}
}) ;
}
}
return ret;
}

The idea is that you separate out the polymorphism bit and the bit where
you actually do something differently depending on class. None of the
stuff you add to RegulatedNME and its implementations is specific to
constraints - it's completely generic. You confine the code that's about
constraints to an implementation of the handler interface. If there are
other places in your app where you do a similar set of instanceof tests,
you can reuse the same dispatch mechanism there.

In this example, i've made the handler an anonymous local class, but
there's no need for it to be that way. It could be a named top-level
class.

Yet another way to do this would be with a trendy modern dynamic
codey-wodey sort of approach. Something like:

interface ConstraintFactory {
public void makeConstraints(RegulatedNME rnme, List<AbstractConstraint> constraints) ;
}

class GateConstraintFactory implements ConstraintFactory {
public void makeConstraints(RegulatedNME rnme, List<AbstractConstraint> constraints) {
Gate gate = (Gate)rnme ;
constraints.add(new UnctrlGateConstraint(gate)) ;
constraints.add(new AnotherUnctrlGateConstraint(gate)) ;
}
}

// likewise for Pump and Weir

class ConstraintMaker {
private Map<Class<RegulatedNME>, ConstraintFactory> factories ;
public ConstraintMaker() {
factories = new HashMap<Class<RegulatedNME>, ConstraintFactory>() ;
factories.put(Gate.class, new GateConstraintFactory()) ;
factories.put(Pump.class, new PumpConstraintFactory()) ;
factories.put(Weir.class, new WeirConstraintFactory()) ;
}
public List<AbstractConstraint> makeConstraints(RegulatedNME rnme) {
List<AbstractConstraint> constraints = new ArrayList<AbstractConstraint>() ;
if (rnme.isGloballyCtrl()) {
constraints.add((new GloballyCtrlConstraint(regNME)) ;
constraints.add(new AnotherGloballyCtrlConstraint(regNME)) ;
}
else if (regNME.isLocallyCtrl()) {
constraints.add(new LocallyCtrlConstraint(regNME)) ;
}
else if (regNME.isUnctrl()) {
constraints.add(new AnotherConstraint(regNME)) ;
ConstraintFactory factory = factories.get(rnme.getClass()) ;
factory.makeConstraints(rnme, constraints) ;
}
return constraints ;
}
}

The trouble with this is that if you introduce a new subclass of
RegulatedNME, it will still compile, but will fail at runtime. And it's
pretty verbose. In languages with first-class functions, it can be a lot
cleaner.

tom

[1] I can't type cedillas - please accept my apologies!

 

[Daniel Pitts]

Hi everyone

I have the following problem. I'm developing a factory method that creates
objects from a list. This list contains objects that implements my interface
IRegulatedNME. Three classes implements this interface : Gate, Pump and
Weir. My factory method looks like this (I hope this code is clear enough) :

private List<AbstractConstraint> create(List<IRegulatedNME> aRegNMEList)
{
List<AbstractConstraint> ret = new ArrayList<AbstractConstraint>();

for (IRegulatedNME regNME : aRegNMEList)
{
if (regNME.isGloballyCtrl())
{
ret.add(new GloballyCtrlConstraint(regNME);
}
else if (regNME.isLocallyCtrl())
{
ret.add(new LocallyCtrlConstraint(regNME);
}
else if (regNME.isUnctrl())
{
if (regNME instanceof Gate)
{
ret.add(new UnctrlGateConstraint((Gate) regNME);
}
else if (regNME instanceof Pump)
{
ret.add(new UnctrlPumpConstraint((Pump) regNME);
}
else // if (regNME instanceof Weir)
{
ret.add(new UnctrlWeirConstraint((Weir) regNME);
}
}
}

return ret;
}

As you could see, the cases "regNME.isGloballyCtrl()" or
"regNME.isLocallyCtrl()" apply to the abstract object. When
"regNME.isUnctrl()", then the constraints to be added depends on the type of
regNME (Gate, Pump or Weir). The list "aRegNMEList" may contain gates, pumps
or weirs at the same time. Is there a way to avoid the use of "instanceof"
in that situation? Could the generics be useful to solve this problem? Is
there a way to filter my list to extract only one type at a time (without
using "instanceof" of course)?

I googled "replace instanceof java" and "filter collection java" but was not
satisfied with the answers.

Thanks in advance for your replies.

François

This whole thing can be simplified:

ret.add(regNME.createConstraint());

This is what polymorphism is for

 

[Daniel Pitts]

how about getListOfConstraints() then?

for (BaseConstraint bc : regNME.getListOfConstraints() ) ret.add(bc);

or ret.addAll(regNME.getListOfConstraints());

 

[Stefan Rybacki]

One minor tiny niggle: in java, it's not conventional to stick an 'I' on
the front of an interface name. RegulatedNME is fine. And if you can
find a proper word that can be used instead of NME, that would be
beautiful!

how about getListOfConstraints() then?

Yup. Personally, i'd call it getConstraints(), and have it return a Set,
unless there's an ordering on constraints. Or it could return a Collection.

Francois [1], you might well not like this solution, but it's definitely
the Official Orthodox Object-Oriented solution. Not that i'm saying it's
the right solution - you might see constraints as being a separate
concern from whatever it is that RegulatedNMEs are mostly about.

for (BaseConstraint bc : regNME.getListOfConstraints() ) ret.add(bc);

If you cannot add it to regNME, then there is hardly any way to avoid
those various instanceof's.

Oh, there is another one: add an enumeration with all the regNME types
and do a switch on regNME.myType()

That's not a bad idea.

While we're on the subject, i'd also use an enumeration for the control
state; rather than those three boolean isXXXCtrl(), methods, define:

enum Control {
GLOBAL,
LOCAL,
UNCONTROLLED
}

And have a method:

public Control getControl() ;

Then instead of the if-else-if cascade, do:

switch (regNME.getControl()) {
case GLOBAL:
// etc
case LOCAL:
// etc
case UNCONTROLLED:
// etc
}

Anyway, back to the question of replacing the instanceof. There is
another way: bounce dispatch. This is an arcane technique of the OO
masters, which i will now share with you. It goes by many names - i
don't think it has a standard one. Most people will know it from the
Visitor pattern, but it's not the same as Visitor - Visitor is this plus
the Internal Iterator pattern.

The key is an interface like this:

interface RegulatedNMEHandler {
public void handle(Gate gate) ;
public void handle(Pump pump) ;
public void handle(Weir weir) ;
}

You then add a method to RegulatedNME:

interface RegulatedNME {
public void accept(RegulatedNMEHandler hdlr) ;
}

With implementations that look like this:

class Gate implements RegulatedNME {
public void accept(RegulatedNMEHandler hdlr) {
hdlr.handle(this) ;
}
}

You now write your method like this:

private List<AbstractConstraint> create(List<RegulatedNME> aRegNMEList)
{
final List<AbstractConstraint> ret = new
ArrayList<AbstractConstraint>();
for (RegulatedNME regNME : aRegNMEList)
{
if (regNME.isGloballyCtrl())
{
ret.add(new GloballyCtrlConstraint(regNME));
ret.add(new AnotherGloballyCtrlConstraint(regNME));
}
else if (regNME.isLocallyCtrl())
{
ret.add(new LocallyCtrlConstraint(regNME));
}
else if (regNME.isUnctrl())
{
ret.add(new AnotherConstraint(regNME));
regNME.accept(new RegulatedNMEHandler() {
public void handle(Gate gate) {
ret.add(new UnctrlGateConstraint(gate);
ret.add(new AnotherUnctrlGateConstraint(gate);
}
public void handle(Pump pump) {
ret.add(new UnctrlPumpConstraint(pump);
}
public void handle(Weir weir) {
ret.add(new UnctrlWeirConstraint(weir);
}
}) ;
}
}
return ret;
}

The idea is that you separate out the polymorphism bit and the bit where
you actually do something differently depending on class. None of the
stuff you add to RegulatedNME and its implementations is specific to
constraints - it's completely generic. You confine the code that's about
constraints to an implementation of the handler interface. If there are
other places in your app where you do a similar set of instanceof tests,
you can reuse the same dispatch mechanism there.

In this example, i've made the handler an anonymous local class, but
there's no need for it to be that way. It could be a named top-level class.

I don't really like this approach because it means you have to adapt the
interface whenever you add a new Constraint type which in return means you need
to adjust all implemented to that changes.

Yet another way to do this would be with a trendy modern dynamic
codey-wodey sort of approach. Something like:

interface ConstraintFactory {
public void makeConstraints(RegulatedNME rnme,
List<AbstractConstraint> constraints) ;
}

class GateConstraintFactory implements ConstraintFactory {
public void makeConstraints(RegulatedNME rnme,
List<AbstractConstraint> constraints) {
Gate gate = (Gate)rnme ;
constraints.add(new UnctrlGateConstraint(gate)) ;
constraints.add(new AnotherUnctrlGateConstraint(gate)) ;
}
}

// likewise for Pump and Weir

class ConstraintMaker {
private Map<Class<RegulatedNME>, ConstraintFactory> factories ;
public ConstraintMaker() {
factories = new HashMap<Class<RegulatedNME>, ConstraintFactory>() ;
factories.put(Gate.class, new GateConstraintFactory()) ;
factories.put(Pump.class, new PumpConstraintFactory()) ;
factories.put(Weir.class, new WeirConstraintFactory()) ;
}
public List<AbstractConstraint> makeConstraints(RegulatedNME rnme) {
List<AbstractConstraint> constraints = new
ArrayList<AbstractConstraint>() ;
if (rnme.isGloballyCtrl()) {
constraints.add((new GloballyCtrlConstraint(regNME)) ;
constraints.add(new AnotherGloballyCtrlConstraint(regNME)) ;
}
else if (regNME.isLocallyCtrl()) {
constraints.add(new LocallyCtrlConstraint(regNME)) ;
}
else if (regNME.isUnctrl()) {
constraints.add(new AnotherConstraint(regNME)) ;
ConstraintFactory factory = factories.get(rnme.getClass()) ;
factory.makeConstraints(rnme, constraints) ;
}
return constraints ;
}
}

The trouble with this is that if you introduce a new subclass of
RegulatedNME, it will still compile, but will fail at runtime. And it's
pretty verbose. In languages with first-class functions, it can be a lot
cleaner.

I like the idea of using factories and a global registry though, because that is
what we use over here in some cases as well and it is powerful, easy to maintain
und to extend you can even drive the registry using reflection or by external
files (factory.xml -> pointing to a factory to register) that can be scanned on
startup.

tom

[1] I can't type cedillas - please accept my apologies!

 

[Hendrik Maryns]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Stefan Rybacki schreef:

Thanks for your reply. Effectively, I don't like the idea of adding
a method
getConstraint() in IRegulatedNME.

One minor tiny niggle: in java, it's not conventional to stick an 'I'
on the front of an interface name. RegulatedNME is fine. And if you
can find a proper word that can be used instead of NME, that would be
beautiful!

I forgot to mention that I may construct more than one constraint at
a time.
My example should be :

how about getListOfConstraints() then?

Yup. Personally, i'd call it getConstraints(), and have it return a
Set, unless there's an ordering on constraints. Or it could return a
Collection.

Francois [1], you might well not like this solution, but it's
definitely the Official Orthodox Object-Oriented solution. Not that
i'm saying it's the right solution - you might see constraints as
being a separate concern from whatever it is that RegulatedNMEs are
mostly about.

ret.add(new GloballyCtrlConstraint(regNME);
ret.add(new AnotherGloballyCtrlConstraint(regNME);
for (BaseConstraint bc : regNME.getListOfConstraints() ) ret.add(bc);

If you cannot add it to regNME, then there is hardly any way to avoid
those various instanceof's.

Oh, there is another one: add an enumeration with all the regNME
types and do a switch on regNME.myType()

That's not a bad idea.

While we're on the subject, i'd also use an enumeration for the
control state; rather than those three boolean isXXXCtrl(), methods,
define:

enum Control {
GLOBAL,
LOCAL,
UNCONTROLLED
}

And have a method:

public Control getControl() ;

Then instead of the if-else-if cascade, do:

switch (regNME.getControl()) {
case GLOBAL:
// etc
case LOCAL:
// etc
case UNCONTROLLED:
// etc
}

Anyway, back to the question of replacing the instanceof. There is
another way: bounce dispatch. This is an arcane technique of the OO
masters, which i will now share with you. It goes by many names - i
don't think it has a standard one. Most people will know it from the
Visitor pattern, but it's not the same as Visitor - Visitor is this
plus the Internal Iterator pattern.

The key is an interface like this:

interface RegulatedNMEHandler {
public void handle(Gate gate) ;
public void handle(Pump pump) ;
public void handle(Weir weir) ;
}

You then add a method to RegulatedNME:

interface RegulatedNME {
public void accept(RegulatedNMEHandler hdlr) ;
}

With implementations that look like this:

class Gate implements RegulatedNME {
public void accept(RegulatedNMEHandler hdlr) {
hdlr.handle(this) ;
}
}

You now write your method like this:

private List<AbstractConstraint> create(List<RegulatedNME> aRegNMEList)
{
final List<AbstractConstraint> ret = new
ArrayList<AbstractConstraint>();
for (RegulatedNME regNME : aRegNMEList)
{
if (regNME.isGloballyCtrl())
{
ret.add(new GloballyCtrlConstraint(regNME));
ret.add(new AnotherGloballyCtrlConstraint(regNME));
}
else if (regNME.isLocallyCtrl())
{
ret.add(new LocallyCtrlConstraint(regNME));
}
else if (regNME.isUnctrl())
{
ret.add(new AnotherConstraint(regNME));
regNME.accept(new RegulatedNMEHandler() {
public void handle(Gate gate) {
ret.add(new UnctrlGateConstraint(gate);
ret.add(new AnotherUnctrlGateConstraint(gate);
}
public void handle(Pump pump) {
ret.add(new UnctrlPumpConstraint(pump);
}
public void handle(Weir weir) {
ret.add(new UnctrlWeirConstraint(weir);
}
}) ;
}
}
return ret;
}

The idea is that you separate out the polymorphism bit and the bit
where you actually do something differently depending on class. None
of the stuff you add to RegulatedNME and its implementations is
specific to constraints - it's completely generic. You confine the
code that's about constraints to an implementation of the handler
interface. If there are other places in your app where you do a
similar set of instanceof tests, you can reuse the same dispatch
mechanism there.

In this example, i've made the handler an anonymous local class, but
there's no need for it to be that way. It could be a named top-level
class.

I don't really like this approach because it means you have to adapt the
interface whenever you add a new Constraint type which in return means
you need to adjust all implemented to that changes.

The complexity has to be somewhere though. Right now, the problematic
part is the instanceofs, this way, you shift it to the Handler
interface. But since you control that interface, it is much easier to
do that. In any case, you need *some* place to keep track of the
complexity.

2¢, H.
- --
Hendrik Maryns
http://tcl.sfs.uni-tuebingen.de/~hendrik/
==================
Ask smart questions, get good answers:
http://www.catb.org/~esr/faqs/smart-questions.html
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.9 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org

iEYEARECAAYFAkk4GOwACgkQBGFP0CTku6MnDQCcDzYXXTHkL5QmDeiFbrH2RNAq
7HYAn0Rg6th1XKFZgcdCZZBsJCraWqCB
=2K2U
-----END PGP SIGNATURE-----

 

[Tom Anderson]

I don't really like this approach because it means you have to adapt the
interface whenever you add a new Constraint type which in return means
you need to adjust all implemented to that changes.

No, you need to adjust it when you add a new RegulatedNME type. And
whatever your solution, you need to adjust things when you do that. It has
no connection to the constraint types at all.

I like the idea of using factories and a global registry though, because
that is what we use over here in some cases as well and it is powerful,
easy to maintain und to extend you can even drive the registry using
reflection or by external files (factory.xml -> pointing to a factory to
register) that can be scanned on startup.

It's no more powerful than any other approach suggested, i think it's far
harder to maintain than the ones which exploit static typing, and i don't
see how it's any easier to make configurable than one of the static
solutions. For example, you could easily write a handler implementation
which read its list of constraints from a config file, rather than having
them hardcoded.

tom

 

[François Grondin]

One minor tiny niggle: in java, it's not conventional to stick an 'I' on
the front of an interface name. RegulatedNME is fine. And if you can find
a proper word that can be used instead of NME, that would be beautiful!

I agree that 'I' may not be conventional, but I work for a group of software
developers that chose to prefix interfaces with 'I'. And 'NME' is short for
'Network Modeling Element', which is meaningful in my area (real-time
control of sewer networks).

how about getListOfConstraints() then?

Yup. Personally, i'd call it getConstraints(), and have it return a Set,
unless there's an ordering on constraints. Or it could return a
Collection.

Francois [1], you might well not like this solution, but it's definitely
the Official Orthodox Object-Oriented solution. Not that i'm saying it's
the right solution - you might see constraints as being a separate concern
from whatever it is that RegulatedNMEs are mostly about.

I'm aware of that. But doing that in my context creates a circular
dependancy between two packages... That's why I didn't want to go that way.

for (BaseConstraint bc : regNME.getListOfConstraints() ) ret.add(bc);

If you cannot add it to regNME, then there is hardly any way to avoid
those various instanceof's.

Oh, there is another one: add an enumeration with all the regNME types
and do a switch on regNME.myType()

That's not a bad idea.

While we're on the subject, i'd also use an enumeration for the control
state; rather than those three boolean isXXXCtrl(), methods, define:

enum Control {
GLOBAL,
LOCAL,
UNCONTROLLED
}

And have a method:

public Control getControl() ;

Then instead of the if-else-if cascade, do:

switch (regNME.getControl()) {
case GLOBAL:
// etc
case LOCAL:
// etc
case UNCONTROLLED:
// etc
}

Anyway, back to the question of replacing the instanceof. There is another
way: bounce dispatch. This is an arcane technique of the OO masters, which
i will now share with you. It goes by many names - i don't think it has a
standard one. Most people will know it from the Visitor pattern, but it's
not the same as Visitor - Visitor is this plus the Internal Iterator
pattern.

The key is an interface like this:

interface RegulatedNMEHandler {
public void handle(Gate gate) ;
public void handle(Pump pump) ;
public void handle(Weir weir) ;
}

You then add a method to RegulatedNME:

interface RegulatedNME {
public void accept(RegulatedNMEHandler hdlr) ;
}

With implementations that look like this:

class Gate implements RegulatedNME {
public void accept(RegulatedNMEHandler hdlr) {
hdlr.handle(this) ;
}
}

You now write your method like this:

private List<AbstractConstraint> create(List<RegulatedNME> aRegNMEList)
{
final List<AbstractConstraint> ret = new
ArrayList<AbstractConstraint>();
for (RegulatedNME regNME : aRegNMEList)
{
if (regNME.isGloballyCtrl())
{
ret.add(new GloballyCtrlConstraint(regNME));
ret.add(new AnotherGloballyCtrlConstraint(regNME));
}
else if (regNME.isLocallyCtrl())
{
ret.add(new LocallyCtrlConstraint(regNME));
}
else if (regNME.isUnctrl())
{
ret.add(new AnotherConstraint(regNME));
regNME.accept(new RegulatedNMEHandler() {
public void handle(Gate gate) {
ret.add(new UnctrlGateConstraint(gate);
ret.add(new AnotherUnctrlGateConstraint(gate);
}
public void handle(Pump pump) {
ret.add(new UnctrlPumpConstraint(pump);
}
public void handle(Weir weir) {
ret.add(new UnctrlWeirConstraint(weir);
}
}) ;
}
}
return ret;
}

The idea is that you separate out the polymorphism bit and the bit where
you actually do something differently depending on class. None of the
stuff you add to RegulatedNME and its implementations is specific to
constraints - it's completely generic. You confine the code that's about
constraints to an implementation of the handler interface. If there are
other places in your app where you do a similar set of instanceof tests,
you can reuse the same dispatch mechanism there.

In this example, i've made the handler an anonymous local class, but
there's no need for it to be that way. It could be a named top-level
class.

Yet another way to do this would be with a trendy modern dynamic
codey-wodey sort of approach. Something like:

interface ConstraintFactory {
public void makeConstraints(RegulatedNME rnme, List<AbstractConstraint>
constraints) ;
}

class GateConstraintFactory implements ConstraintFactory {
public void makeConstraints(RegulatedNME rnme, List<AbstractConstraint>
constraints) {
Gate gate = (Gate)rnme ;
constraints.add(new UnctrlGateConstraint(gate)) ;
constraints.add(new AnotherUnctrlGateConstraint(gate)) ;
}
}

// likewise for Pump and Weir

class ConstraintMaker {
private Map<Class<RegulatedNME>, ConstraintFactory> factories ;
public ConstraintMaker() {
factories = new HashMap<Class<RegulatedNME>, ConstraintFactory>() ;
factories.put(Gate.class, new GateConstraintFactory()) ;
factories.put(Pump.class, new PumpConstraintFactory()) ;
factories.put(Weir.class, new WeirConstraintFactory()) ;
}
public List<AbstractConstraint> makeConstraints(RegulatedNME rnme) {
List<AbstractConstraint> constraints = new
ArrayList<AbstractConstraint>() ;
if (rnme.isGloballyCtrl()) {
constraints.add((new GloballyCtrlConstraint(regNME)) ;
constraints.add(new AnotherGloballyCtrlConstraint(regNME)) ;
}
else if (regNME.isLocallyCtrl()) {
constraints.add(new LocallyCtrlConstraint(regNME)) ;
}
else if (regNME.isUnctrl()) {
constraints.add(new AnotherConstraint(regNME)) ;
ConstraintFactory factory = factories.get(rnme.getClass()) ;
factory.makeConstraints(rnme, constraints) ;
}
return constraints ;
}
}

The trouble with this is that if you introduce a new subclass of
RegulatedNME, it will still compile, but will fail at runtime. And it's
pretty verbose. In languages with first-class functions, it can be a lot
cleaner.

tom

[1] I can't type cedillas - please accept my apologies!

Tom, I thank you very much for your last two solutions. This is exactly what
I'm looking for. I'll probably go with the Visitor-like pattern.

BTW, no problem with the cedilla. And also, thanks for your reply to Stefan
Ram. He was right about the parentheses, but as you mentioned, the lines
were understandable. You read my mind.

Best regards.

Francois