17

In our Rails application, we are adding notifications. Some of these are blocking: They stop the progress of whatever resource they are added on, because some information on that resource is missing.

Other notifications are simple notifications, and only provide information.

Today I had a discussion with another programmer on our team. I have created the inheritance structure like this:

enter image description here

He however, would rather like me to just add blocking as a boolean-returning method on each Notification, and specify a list of subclasses that are blocking inside the Notification parent class.

The difference between these approaches is not very large; in my approach one does not have to specify this list, keeping the root class cleaner. On the other hand, the special logic that happens in Notification::Blocking right now is not very large either.

What kind of abstraction is more suited for this problem?

  • 11
    A parent class should never know about it's children. Why do you need to keep a list of sub classes? – coteyr May 23 '16 at 22:07
  • How are they added on a resource and how do they stop its progress? – null May 23 '16 at 23:10
  • 3
    Why do you need so many notification classes? Sounds to me like you could create one notification class, then let the data drive the actions instead of the data types. – Trisped May 24 '16 at 0:29
  • 1
    @Trisped: right, if you find yourself moving one aspect of the behaviour into the base class with an exhaustive list of cases, then have the courage of your convictions, admit that you're not really designing a usable base class for customisation by extension, and move all the behaviour into the base class! – Steve Jessop May 24 '16 at 10:15
35

You want to avoid base classes knowing about derived classes. It introduces tight coupling and is a maintenance headache because you have to remember to add to the list every time you create a new derived class.

It will also prevent you from being able to put the Notification class into a reusable package/assembly if you wanted to use this class in multiple projects.

If you really want to use a single base class, another way to solve this is to add a virtual property or method IsBlocking on the base Notification class. Derived classes could then override that to return true or false. You would have a single class solution without the base class knowing about derived classes.

  • 3
    This. Make decisions in one place. Don't scatter knowledge of which classes block between the class and the list. – candied_orange May 23 '16 at 19:36
  • This is the one I do, works very well and is very reusable. – coteyr May 23 '16 at 22:02
13

and specify a list of subclasses that are blocking inside the Notification parent class.

That looks very peculiar and is a particular code smell.

I would provide subclasses if you have differences in behaviour between the classes, and you want to treat all of these notifications in the same fashion (i.e. using polymorphism).

  • 1
    I think "behaviour" is the key here: when it's just data, the field should be a sufficient discriminator. Behaviour is the better reason to use polymorphism however one should always consider maintenance complexity when creating inheritance hierarchies. Read en.wikipedia.org/wiki/Composition_over_inheritance – cottsak May 24 '16 at 8:15
7

As a converse to the existing answers, I would suggest that the boolean property is the best option if the mode that is to be used is required to be changed dynamically (e.g. via a configuration file that gives a list of which types are to be blocking and which aren't).

That said, a better design even in this situation might be to use a Decorator object.

1

I would say it depends on how much else is special about a blocking notification, though my first thought is to go with "both":

class Notification
 virtual Boolean Blocking{get return false;}

class BlockingNotification inherits Notification
 virtual overrides Boolean Blocking{get return true;}

That way, you can use n.Blocking or n is BlockingNotification (all in pseudo-code), although, if you're going to allow a class to implement a context-sensitive Blocking value, seeing as you'd have to check that value every time, the BlockingNotification class becomes less useful.

In any case, I agree with the other answers that you don't want the base class implementation of Blocking to have to know about the derived classes.

0

Instead of making two base classes and multiple instances of each, make one notification class with a bool to indicate if the notification is blocking and any other information needed to communicate the notification to the user.

This allows you to use one set of code for processing and presenting notifications and reduces the complexity of your code.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.