11

Consider an interface:

interface IWaveGenerator
{
    SoundWave GenerateWave(double frequency, double lengthInSeconds);
}

This interface is implemented by a number of classes which generate waves of different shapes (for example, SineWaveGenerator and SquareWaveGenerator).

I want to implement a class that generates a SoundWave based on musical data, not raw sound data. It would receive the name of a note and a length in terms of beats (not seconds), and internally use the IWaveGenerator functionality to create a SoundWave accordingly.

Question is, should the NoteGenerator contain an IWaveGenerator or should it inherit from an IWaveGenerator implementation?

I'm leaning towards composition because of two reasons:

1- It allows me to inject any IWaveGenerator to the NoteGenerator dynamically. Also, I only need one NoteGenerator class, instead of SineNoteGenerator, SquareNoteGenerator, etc.

2- There's no need for NoteGenerator to expose the lower-level interface defined by IWaveGenerator.

However I'm posting this question to hear other opinions regarding this, maybe points I haven't thought of.

BTW: I would say NoteGenerator is conceptually an IWaveGenerator because it generates SoundWaves.

14

It allows me to inject any IWaveGenerator to the NoteGenerator dynamically. Also, I only need one NoteGenerator class, instead of SineNoteGenerator, SquareNoteGenerator, etc.

That is a clear sign it would be better to use composition here, and not inherit from SineGenerator or SquareGenerator or (worse) both. Nethertheless, it will make sense to inherit a NoteGenerator directly from an IWaveGenerator if you change the latter a little bit.

The real problem here is, it is probably meaningful to have NoteGenerator with a method like

SoundWave GenerateWave(string noteName, double noOfBeats, IWaveGenerator waveGenerator);

but not with a method

SoundWave GenerateWave(double frequency, double lengthInSeconds);

because this interface is too specific. You want IWaveGenerators to be objects which generate SoundWaves, but currently your interface expresses IWaveGenerators are objects which generate SoundWaves from frequency and length exclusively . So better design such an interface this way

interface IWaveGenerator
{
    SoundWave GenerateWave();
}

and pass parameters like frequency or lengthInSeconds, or a completely different set of parameters through the constructors of a SineWaveGenerator, a SquareGenerator, or whatever other generator you have in mind. This will allow you to create other kind of IWaveGenerators with completely different construction parameters. Maybe you want to add a rectangle wave generator which needs a frequency and two length parameters, or something like that, maybe you want to add a triangle wave generator next, also with at least three parameters. Or, a NoteGenerator, with constructor parameters noteName, noOfBeats, and waveGenerator.

So the general solution here is to decouple the input parameters from the output function, and make only the output function part of the interface.

  • Interesting, haven't thought of this. But I wonder: does this (setting the 'parameters to a polymorphic function' in the constructor) often work in reality? Because then the code would indeed have to know what type it's dealing with, thus ruining Polymorphism. Can you give an example where this would work? – Aviv Cohn Jun 19 '15 at 14:20
  • 2
    @AvivCohn: "the code would indeed have to know what type it's dealing with" - no, that is a misconception. Only the part of the code which constructs the specific type of generator (mybe a factory), and that has to know always which type it's dealing with. – Doc Brown Jun 19 '15 at 20:42
  • ... and if you need to make the construction process of your objects polymorphic, you can use the "abstract factory" pattern (en.wikipedia.org/wiki/Abstract_factory_pattern) – Doc Brown Jun 19 '15 at 20:48
  • This is the solution I would pick. Small, immutable classes is the right way to go here. – Stephen Aug 10 '15 at 23:58
9

Whether or not NoteGenerator is "conceptually" an IWaveGenerator does not matter.

You should only inherit from an interface if you plan on implementing that exact interface according to the Liskov Substitution Principle, i.e. with the correct semantics as well as the correct syntax.

It sounds like your NoteGenerator might have syntactically the same interface, but its semantics (in this case, the meanings of the parameters it takes) will be very different, so using inheritance in this case would be highly misleading and potentially error-prone. You're right to prefer composition here.

  • Actually I didn't mean NoteGenerator would implement GenerateWave but interpret the parameters differently, yes I agree that would be a terrible idea. I meant NoteGenerator is kind of a specialization of a wave generator: it's able to take in 'higher level' input data instead of just raw sound data (e.g. a note name instead of a frequency). I.e. sineWaveGenerator.generate(440) == noteGenerator.generate("a4"). So there comes the question, composition or inheritance. – Aviv Cohn Jun 19 '15 at 0:02
  • If you can come up with a single interface that fits both the high and low level wave generation classes, then inheritance can be acceptable. But that seems very difficult, and unlikely to have any real benefits. Composition definitely seems like the more natural choice. – Ixrec Jun 19 '15 at 0:04
  • @Ixrec: actually, it is not very difficult to have a single interface for all types of generators, the OP should probably do both, use composition to inject a low level generator and inherit from a simplified interface (but not inherit the NoteGenerator from a low level generator implementation) See my answer. – Doc Brown Jun 19 '15 at 12:41
5

2- There's no need for NoteGenerator to expose the lower-level interface defined by IWaveGenerator.

Sounds like NoteGenerator is not a WaveGenerator, so therefore should not implement the interface.

Composition is the correct choice.

  • I would say NoteGenerator is conceptually an IWaveGenerator because it generates SoundWaves. – Aviv Cohn Jun 18 '15 at 23:37
  • 1
    Well, if it doesn't need to expose GenerateWave, then it's not an IWaveGenerator. But it sounds like it uses a IWaveGenerator (maybe more?), hence composition. – Eric King Jun 18 '15 at 23:45
  • @EricKing: this is a correct answer as long as one has to stick to the GenerateWave function as it is written in the question. But from the comment above I guess that is not what the OP really had in mind. – Doc Brown Jun 19 '15 at 12:47
3

You have a solid case for composition. You may have a case to also add inheritance. The way to tell is by looking at the calling code. If you want to be able to use a NoteGenerator in existing calling code that expects an IWaveGenerator, then you need to implement the interface. You're looking for a need for substitutability. Whether it conceptually "is-a" wave generator is beside the point.

  • In that case, i.e. choosing composition, but still needing that inheritance in order to make substitutability happen, the "inheritance" would be named e.g. IHasWaveGenerator, and the relevant method on that interface would be GetWaveGenerator which returns an instance of IWaveGenerator. Of course naming can be changed. (I'm just trying to flesh out more details - let me know if my details are wrong.) – rwong Feb 10 '17 at 8:13
2

It is fine for NoteGenerator to implement the interface, and also, for NoteGenerator to have an internal implementation that references (by composition) another IWaveGenerator.

Generally, composition results in more maintainable (i.e. readable) code, because you don't have complexities of overrides to reason over. Your observation about the matrix of classes you'd have when using inheritance is also on point, and can probably be thought of as a code smell pointing toward composition.

Inheritance is better used when you have an implementation that you want to specialize or customize, which doesn't seem to be the case here: you just need to use the interface.

  • 1
    It is not OK for NoteGenerator to implement IWaveGenerator because because notes require beats. not seconds-. – Tulains Córdova Jun 19 '15 at 13:32
  • Yes, certainly if there is no sensible implementation of the interface, then the class should not implement it. However, the OP did state that "I would say NoteGenerator is conceptually an IWaveGenerator because it generates SoundWaves", and, he was considering inheritance, so I took mental latitude for the possibility that there might be some implementation of the interface, even though there is another better interface or signature for the class. – Erik Eidt Jun 19 '15 at 17:20

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.