OO design question

[Daniel Billingsley]

Can someone please comment if there's something wrong from an OO point of
view with the model represented by the following code snippet. I hope it's
fairly obvious what the point is of the code I haven't explicitly included.

For purposes of this discussion let's say that classes derived from
SomeBaseClass my implement any combination of several interfaces.
Therefore, I'm not crazy about having a bunch of derived classes with all
possible combinations of them between SomeBaseClass and my final
instantiated class.

I suppose that SomeBaseClass shouldn't really have any idea what interfaces
a derived class might implement to be OO pure, but I also don't want to
repeat this code in all the derived classes. Maybe there is a better
solution??

=======================

class SomeBaseClass : SomeRootClass
{

protected void hookupChildListeners()
{
if (this is IManager)
{
((IManager)this).DirectReports.ListChanged += new
System.ComponentModel.ListChangedEventHander(childListChanged);
}
if (this is IBiologicalParent)
{
((IBiologicalParent)this).Children.ListChanged += new
System.ComponentModel.ListChangedEventHander(childListChanged);
}
if (this is IFosterParent)
{
((IFosterParent)this).Children.ListChanged += new
System.ComponentModel.ListChangedEventHander(childListChanged);
}
/// Etc. etc.
}

private void childListChanged(object sender,
System.ComponentModel.ListChangedEventArgs e)
{
onSomethingChanged(); // provides notification to interested
listeners that this object has changed.
}

}
-----------------------------

class SomeChildClass : SomeBaseClass, IBiologicalParent, IManager
{
private setup()
{
hookupChildListeners();
}
}

 

[Nathan Neitzke]

Daniel,
All I can say is that from an OO perspective, this is not a good solution.
You really should be hooking up the event in the derived classes.

You can still provide the method in the base class that performs that
functionality, but hooking it up to an event should occur in the child class
since that is where the interface is inherited.

Otherwise that is going to be a very ugly base class.

Take care.

 

[Alexander Shirshov]

Daniel,

You could use Reflection or TypeDescriptor.

It looks like the code in childListChanged performs generic tasks and it
doesn't need to know whether an object it receive notifications from
implements a specific interface.

You can enumerate all properties and subscribe to events of those objects
that implement IBindingList:

abstract clall SomeBaseClass
{
public setup()
{
foreach (PropertyInfo propInfo in this.GetType().GetProperties())
{
if
(propInfo.PropertyType.IsAssignableFrom(typeof(IBindingList))) // or is it
typeof(IBindingList).IsAssignableFrom(propInfo.PropertyType)?
{
IBindingList obj = propInfo.GetValue(this, null);
obj.ListChanged += new
..ListChangedEventHander(childListChanged); // It should be "plus equals". OE
eats my plus signs!
}
}
}
}

This will effectively hook events of DirectReports and Children.


HTH,
Alexander

 

[Bernhard Volz]

Daniel and Nathan,

Daniel,
All I can say is that from an OO perspective, this is not a good solution.
You really should be hooking up the event in the derived classes.

You can still provide the method in the base class that performs that
functionality, but hooking it up to an event should occur in the child class
since that is where the interface is inherited.

Otherwise that is going to be a very ugly base class.

Take care.

I agree with that. One practical solution would be to make a method
which actually performs the hookup of the event handlers abstract in the
base class and override it in the derived classes.

abstract class SomeBaseClass : RootClass
{
protected abstract void hookupChildListeners();

protected void childListChanged(
object sender,
System.ComponentModel.ListChangedEventArgs e)
{
// notifications
}
}

class IManagerImplementation : SomeBaseClass
{
protected override void hookupChildListeners()
{
this.DirectReports.ListChanged += new
System.ComponentModel.ListChangedEventHander(childListChanged);
}

}

 

[Nathan Neitzke]

Volz,
Good stuff, you made mine even better!

 

[Joanna Carter \(TeamB\)]

I agree with that. One practical solution would be to make a method
which actually performs the hookup of the event handlers abstract in the
base class and override it in the derived classes.

May I just add that this is the perfect and correct solution. If you start
doing code in a base class, conditional on which class you really are in;
then this is known as a 'bad smell' and is easily rectified by this
technique, sometimes variously known as either the Strategy or Template
Method design pattern.

Joanna

 

[Daniel Billingsley]

Yeah I know I was smelling something pretty bad, hence my post. I certainly
considered this type of solution, but my problem is that while indeed it
"easily rectifies" the bad OO design it does nothing to help with having the
same basic code repeated in hundreds of derived classes. But I'll accept
that this is a case where OO properness / purity just has the cost of
additional lines of code.

 

[Daniel Billingsley]

Now that's an approach I should have thought of. You're correct, all
SomeBaseClass needs to know there is when a child collection has changed,
which means transitively that interested parties should be notified that IT
has changed.

I'll have to give it some thought for my particular need. Given the
variable timing of when the various child collections may be created/loaded
the abstract method type of approach others have suggested may end up being
significantly more straightforward.

Thanks everyone.

 

[Joanna Carter \(TeamB\)]

Yeah I know I was smelling something pretty bad, hence my post. I certainly
considered this type of solution, but my problem is that while indeed it
"easily rectifies" the bad OO design it does nothing to help with having the
same basic code repeated in hundreds of derived classes. But I'll accept
that this is a case where OO properness / purity just has the cost of
additional lines of code.

To avoid writing almost the same code then only use a template method to get
the event that needs assigning to.

/////////////////////

public class BaseClass
{
protected abstract ListChangedEventHandler GetListChangedEvent();

protected void hookupChildListeners()
{
ListChangedEventHandler theEvent = GetListChangedEvent();
theEvent += new ListChangedEventHander(childListChanged);
}
}

public class DerivedClass : BaseClass
{
protected override ListChangedEventHandler GetListChangedEvent()
{
return DirectReports.ListChanged;
// or whatever it takes to get the event from this derived class
}
}

////////////////////

Joanna

--
Joanna Carter (TeamB)

Consultant Software Engineer
TeamBUG support for UK-BUG
TeamMM support for ModelMaker

 

[Daniel Billingsley]

There's still identical code in the derived classes. And it would need some
modification as there may be more than one list.

But we've confirmed my suspicion that my original approach was fowl, and I
get the gist of the alternatives.

Thanks again.

 

[Joanna Carter \(TeamB\)]

There's still identical code in the derived classes. And it would need some
modification as there may be more than one list.

Well, if the code is truly identical, then move it back to the base class.

The code I gave llows you to change the abstract 'getter' method to virual
and place code there as well.

Can you show us where you see the duplication happening if you used my
template method ?

Joanna

--
Joanna Carter (TeamB)

Consultant Software Engineer
TeamBUG support for UK-BUG
TeamMM support for ModelMaker

 

[Daniel Billingsley]

Joannna:

Well, if the code is truly identical, then move it back to the base class.

It's identical for each of the same *type* of derived class, but would
differ between derived classes that had different child lists.

Note that I already mentioned the possibility of having an additional class
between the base class and the final derived class for each *type*, but as I
would need to represent all possible permutations having ListAContainerBase,
ListAContainerAndListBContainerBase, ListAContainerAndListCContainerBase,
etc. etc. would kind of messy from a maintenance standpoint.

Can you show us where you see the duplication happening if you used my
template method ?

Yeah, this code

protected override ListChangedEventHandler GetListChangedEvent()
{
return DirectReports.ListChanged;
// or whatever it takes to get the event from this derived class
}

would be repeated in every derived class that had a DirectReports list.

 

[Bernhard Volz]

Daniel:

would be repeated in every derived class that had a DirectReports list.

Well, you certainly should spread the code to different classes. This is
the "cost" of the paradigm. But when I understood it right, you only
have to introduce another layer in your class hierarchy which does the
adding of the event handler. The derived classes in your example would
then not directly derive from your base class but from this new layer -
according to the type needed.

I mean something like this (not real code!):

abstract class Base
{
protected abstract void addHandler();
}

abstract class Derived1 : Base, Interface1
{
protected override void addHandler()
{
// add the event handler specific to Interface1
}

// rest of methods of Interface1 is declared abstract
}

class Child1 : Derived1
{
// interface methods of Interface1 (declared as override)
}

For every combination of your types you now need a class DerivedX
implementing the addHandler() method specific to the implemented
interface. I know that this looks a bit strange but by using such a
construct you don't have to repeat exactly the same code many times.

Hope I could help,
Bernhard

 

[Bernhard Volz]

Nathan:

Good stuff, you made mine even better!

Well, thanks for the flowers

 

[Daniel Billingsley]

Well, you certainly should spread the code to different classes. This is

the "cost" of the paradigm.

Yes, as I said several posts ago, that is a "cost" I accept.

For every combination of your types you now need a class DerivedX

Yes, that's what I said I don't like. Make it more than a couple of
interfaces and it looks to me like a mantinenance fiasco waiting to happen.
Personal opinion I guess, but this is the least attractive of the possible
solutions to me.