6

I'm working on an application that creates music by itself. I'm seeking feedback for my OO design so far. This question will focus on one part of the program.


The application produces Tune objects, that are the final musical products. Tune is an abstract class with an abstract method play. It has two subclasses: SimpleTune and StructuredTune.

SimpleTune owns a Melody and a Progression (chord sequence). It's play implementation plays these two objects simultaneously. StructuredTune owns two Tune instances. It's own play plays the two Tunes one after the other according to a pattern (currently only ABAB).

Melody is an abstract class with an abstract play method. It has two subclasses: SimpleMelody and StructuredMelody.

SimpleMelody is composed of an array of notes. Invoking play on it plays these notes one after the other. StructuredMelody is composed of an array of Melody objects. Invoking play on it plays these Melodyies one after the other.

I think you're starting to see the pattern. Progression is also an abstract class with a play method and two subclasses: SimpleProgression and StructuredProgression, each composed differently and played differently.

SimpleProgression owns an array of chords and plays them sequentially. StructuredProgression owns an array of Progressions and it's play implementation plays them sequentially.

enter image description here


Every class has a corresponding Generator class. Tune, Melody and Progression are matched with corresponding abstract TuneGenerator, MelodyGenerator and ProgressionGenerator classes, each with an abstract generate method. For example MelodyGenerator defines an abstract Melody generate method.

Each of the generators has two subclasses, Simple and Structured. So for example MelodyGenerator has a subclasses SimpleMelodyGenerator, with an implementation of generate that returns a SimpleMelody.

(It's important to note that the generate methods encapsulate complex algorithms. They are more than mere factory method. For example SimpleProgressionGenerator.generate() implements an algorithm to compose a series of Chord objects, which are used to instantiate the returned SimpleProgression).

Every Structured generator uses another generator internally. It is a Simple generator be default, but in special cases may be a Structured generator.

enter image description here


Parts of this design are meant to allow the end-user through the GUI to choose what kind of music is to be created. For example the user can choose between a "simple tune" (SimpleTuneGenerator) and a "full tune" (StructuredTuneGenerator). Other parts of the system aren't subject to direct user-control.


My question regarding this design is rather specific:

When I look at my design, it looks like the design-equivalent of 'code duplication': 'class-duplication' or 'design-duplication'. You can clearly see what I'm referring to if you look at the above diagram.

My question is split into two:

  1. Is my intuition correct? Is this duplication, or reoccuring pattern in the design, a bad thing?
  2. If it is, how can I design this differently and more efficiently?
  • 1
    "And then each has a corresponding X class" sets off alarms for me. – Telastyn Aug 19 '14 at 11:45
  • Why does a melody or progression know how to play itself? – Doval Aug 19 '14 at 11:45
  • 1
    @gnat Do you mean I should make it more specific? – Aviv Cohn Aug 19 '14 at 11:47
  • 1
    Because a melody isn't characterized by the mechanism by which it's played. That's akin to making a rectangle that knows how to draw itself on the screen. There's a difference between what something is and what you can do with it. That aside, is there any need for melodies, progressions, and chords to be abstract? They seem like pretty straightforward data objects to me. What implementation details are they trying to protect? – Doval Aug 19 '14 at 11:48
9

Are those really subclasses? I don't think you need all those subclasses.

As you've already pointed out:

SimpleMelody is composed of an array of notes. Invoking play on it plays these notes one after the other. StructuredMelody is composed of an array of Melody objects. Invoking play on it plays these Melodies one after the other.

In other words, SimpleMelody is an Array of Notes, not an "is a" melody. StructuredMelody is an array of melody objects, not an elaboration of Melody.

They are just collections, in other words. If you want to wrap those collections in their own classes and give them additional properties or methods, use composition, not inheritance.

There's nothing wrong with having one-for-one generator classes. If that makes you uncomfortable, put static factory methods in the actual classes, as in

var melody = Melody.Create(foo);

In general, I think you're creating a lot of abstraction for not much benefit. Just stick with Note, Melody and collection classes at first, and add the additional abstractions if you need them.

  • Thanks for answering. I'm trying to understand what you mean. You're saying "If you want to wrap those collections in their own classes and give them additional properties or methods, use composition, not inheritance". What does that mean? (I know what composition means, I'm asking what you mean in this context). Can you give an example of how you'd implement this differently? – Aviv Cohn Aug 20 '14 at 0:10
  • It means that, instead of subclassing Note to get SimpleMelody, just make the Notes[] collection a private member of SimpleMelody, and forget about the subclassing. I'm not sure subclassing Note even makes sense in this context. What would you do with the inheritance relationship that you can't already do with the Notes[] collection? – Robert Harvey Aug 20 '14 at 0:12
  • SimpleMelody isn't a subclasses of Note, it's a subclass of Melody. Nothing is a subclasss of Note. The reason Melody is abstract with two subclasses, SimpleMelody and StructuredMelody, is that I can choose at runtime if I want to create a complex melody with sub-melodies and 'phrasing' (in which case I'll use a StructuredMelodyGenerator, which gives me a StructuredMelody), or if I just want a plain simple melody (in which case I would use a SimpleMelodyGenerator, which gives me a SimpleMelody. – Aviv Cohn Aug 20 '14 at 14:32
  • By the way, Note[] is indeed a private member of SimpleMelody, while Melody[] is a private member of StructuredMelody. – Aviv Cohn Aug 20 '14 at 14:34
  • Please let me rephrase my original question: when I look at my design, it feels like the design-equivalent of code-duplication: "class duplication" or "design duplication". If you look at the diagram, there's a precisely re-occuring pattern. My question can be split into two: 1- Am I right? Is this 'duplication' bad? 2- If so, how can I do this differently? – Aviv Cohn Aug 20 '14 at 14:43
2

I think the fundamental problem with your design is that you're introducing a new type for each way to combine your data types and on top of that making the structure of your songs private.

Imagine if I were making a math library, and I had a type for "simple integers" which are just literals. And then I created a separate type "added integers" for the sum of two simple integers. And then a separate type "multiplied integers" for the multiplication of two added integers. Now on top of that I keep the bit pattern that makes up the numbers a secret so the only mathematical operations that'll ever be possible are the ones hard-coded into my library. That's what you're doing.

I think you could achieve what you want by representing everything as (preferably immutable) sequences of chords and making that sequence public. You can re-implement your generators as plain old functions that duplicate, transform, and glue sequences of chords in particular ways. Alternatively, songs could be lazily-produced streams of chords, which would allow you to generate infinite songs. Either design ends up much simpler and there's no limit to the number of ways you can compose songs.

  • Let me see if I understand: instead of having different Melody types - SimpleMelody which is composed of Notes and StructuredMelody which is composed of Melodys - I'll have only one, concrete Melody class, which is composed of a sequence of Notes. The different generators - i.e. Simple and Structured - still exist, but the only difference between the two is the algorithm they implement to compose the output Melody. At the end, they both output the same concrete Melody type, which again is a plain sequence of Notes. Is this what you mean? – Aviv Cohn Aug 20 '14 at 18:19
  • @Prog Yes. The particulars may vary slightly (I don't know whether Notes or Chords are the fundamental primitive here, since I'm not a music buff) but that's what I meant. – Doval Aug 20 '14 at 18:26
2

Try to think in a big problem its always complicated, i try to focus in a small part, you say:

"SimpleMelody is composed of an array of notes. Invoking play on it plays these notes one after the other. StructuredMelody is composed of an array of Melody objects. Invoking play on it plays these Melodyies one after the other."

I see a lack of abstraction that generates duplication, you can think in the problem in other way:

"Melody is composed of an array of playable objects. Invoking play on it plays these playable objects one after the other"

You don't need two different classes (melody and structuredMelody) to do the same thing. The melody generators can take care of composing the melody with the right "pieces".

Probably with the same approach you can simplify a lot the rest of the system.

2

Design as you talk

As already pointed out, you have some smelly stuff going on.

You say:

StructuredTune owns two Tune instances. It's own play plays the two Tunes one after the other according to a pattern (currently only A-B-A-B)

Your implementation however:

StructuredTune is a Tune that owns 2 tunes.

huh? Wow, euhmm. huh?

Now read the above again but change Tune with Melody.

huh?

Now read it another time, but now replace Tune with Progression

huh?

WTFsPM

Also known as WTFs / minute

And it starts with the big picture. A StructuredMelody knows about Melodies where as a SimpleMelody knows only about Notes.

This would be ok, because now a StructuredMelody could hold multiple StructuredMelodies that each have multiple StructuredMelodies. Cool!

Then we also have a StructuredTune that holds multiple Tunes that each hold a Structured Melody that holds multiple StructuredMelodies that ...

huh?

So a StructuredTune is actually a StructuredMelody. Only instead of having Melodies directly it holds Tunes that hold a Melody. wow! But then now you have the ability to also add Progressions (also known as Chords?).

Now read the same again, but with an extra iteration of those Progressions in them.

I'm lost

No really, I'm a musician and (should) know a lot about music stuff. But you lost me, a long while back.

The wrong pattern for the wrong reason

that is your problem :) You found a cool pattern and re using it everywhere. That's not cool!

Side note: been there done that. I abused the SingletonPattern for over a year, just because I liked it.

The better way

better because nothing is 'best'. We just have better solutions.

First of you should choose an approach. One could go for the pitch+note approach where a Note consists of a pitch and a duration. Or forget about pitch and only use a note and handle duration somewhere else.

You also should decide on how you are using chords. Is a chord a root Pitch with a chordQuality? or some Pitches crammed together. Or Notes?
A chord could have 3 Notes, each with a different duration. yuck :(

Then our progression. Is it some Playable stuff crammed together? Or do we in fact mean a chordProgression that as a rootChord and a progression (i.e. I-IV-V)? Or should we call it Harmony that has a rootTonality with a progression? lost again.

Depending on what you are building, different needs may arrive. You might also go for an approach where chords and progressions don't exist.

Just Notes that have a Pitch an a Duration.

A tune then is a collection of Notes and some kind of knowledge of when to start what note. We don't need chords here ;)

Or even simpler. We could define a Note with a pitch, start-time and stop-time. Stupid simple.

Your names are wrong

Names are important. They are the most important thing in your software. And defining them is as important.

Personally I would use the following approach:

The StartStoppables:

  • A Pitch
  • A Chord has multiple Pitch

Our StartStoppable interface:

interface StartStoppable
{
    public function start();
    public function stop();
}

The Playables:

  • A Note has a Pitch and a Duration
  • A Harmony has Chord and a Duration

Our Playable interface:

interface Playable
{
    public function play();
}

The progression related stuff:

  • A Progression defines a progression strategy (i.e. V-IV-I-I , other progression notations may be used)
  • A ChordProgression has multiple Harmony and is also Playable (!)

We then add some factories to aid us in the creation of our objects.

  • a PitchFactory can create a Pitch given a value and the used notation (i.e. Neo-Latin)
  • a ChordFactory can create a Chord from a given Pitch and chord quality.
  • a HarmonyFactory can create a Harmony from a given Pitch and HarmonySet (i.e. MajorTriad).
  • a HarmonyProgressionFactory can create a HarmonyProgression from a given Chord and a Progression

Our PitchFactory would look something like this:

class PitchFactory
{
    const ENGLISH = 0; //default notation
    const NEOLATINN = 1;

    public static function create(String $value, int $notation=0) {...}
}

And then used as:

$pitch = PitchFactory::crate('DO', PitchFactory::NEOLATIN);
//or
$pitch = PitchFactory::create('C');;

Our ChordFactory could be used as follows:

$chord = ChordFactory::create($pitch, ChordFactory::MAJOR); //Major chord

As noted in other answers, these create methods could be moved into the objects themselves to create a nicer way of coding:

$pitch = Pitch::create('DO', PITCH::NEOLATIN);
$chord = Chord::create($pitch, Chord::MAJOR);

We can then start using our notes and harmonies:

$length = 1; //whatever this means

$note = new Note($pitch, $length);

$harmony = new Harmony($chord, $length);

//or maybe a minor triad with root set to $pitch
$harmony = Harmony::create($pitch, $length, Harmony::MinorTriad);

And then finally our HarmonyProgression:

$progression = new Progression('V-IV-I-I');
$harmonyProgression = HarmonyProgressionFactory::create($chord, $progression);

Later on the notations could be factored into an own class. This will give you flexibility in being able to use Tabular, Roman, etc. notations independent of the rest of your application.

  • +1 I like the way you are thinking, I would have continued I the direction of how an orchestra works. So note - chord - sound / instrument type (horns / strings etc) - section (principle, harmony, accompaniment etc) - orchestra - conductor. Now the model would feel even more natural. There is now lower limit on the number of sections, is if you wanted to model a duet your orchestra would consist of 2 sections each with a single instrument. – DaveM Jun 15 '15 at 7:46
1

It looks like you have seen (or reinvented) the "composite pattern" somewhere, and now you are throwing it at at all of your main objects (tunes, melodies, progression), without knowing if it is really needed or not ("just in case"). This approach has a high risk of overdesigning things. The current design will allow you to model tree-like hierarchies of tunes, melodies and progressions, for which the tree structure of each is completely independent of each other. Do you really need that for your application? Is it necessary to have a simple tune build up from a

  • tree-like hierarchy of melodies, and a second
  • independent tree-like hierarchy of progressions?

Or does combining a structured melody consisting of 3 simple melodies with another melody consisting of 2 simple melodies result in a structured melody of 5 melodies, where the original grouping does not matter any more afterwards? If the latter is the case, I would try to start just with a Tune class, holding a list of melodies and a list of progressions.

And why do you need a complete "tree of tunes"?

And even if you are convinced you need all those hierarchies, it is definitely not clear why the generator hierarchy needs to follow the composite pattern at all. I guess you will need at maximum one generator object of each, so why not create, for example, a MelodyGenerator class, with just one object instance, which will be used to produce many melody objects (which will be either of type SimpleMelody or StructuredMelody). So if you need a "generateSimpleMelody" and a "generateStructuredMelody" method, make it two methods of the generator class, don't introduce an own class whenever you need just to add another method. A complete tree like hierarchy of MelodyGenerator objects seems IMHO to be pretty useless.

Nonetheless, "form follows function", so I would actually try to implement a part of your application, starting with the most simple possible object model you can think of (Robert Harvey's suggestion sounds well, a Melody with an array of notes maybe enough for the first version of your application), and extend your model whenever your application needs to get a feature you cannot solve with simple collection classes.

1

After reading the answers posted to this question, I decided to alter my design. This is what it looks like now (simplified):

enter image description here

In the previous design, Melody and Progression did almost the same thing, except they were playing different elements: Notes or Chords respectively. Creating duplication.

I realized that I can improve the design by giving Note and Chord a common title: Playable. Than I can have a more general class Sequence that plays Playables.

MelodyGenerator and ProgressionGenerator then both instantiate the same kind of object, a Sequence, only they compose it with a different kind of elements: either Notes or Chords.

Also, I gave up on the recursive-tree-kind-of-thing I had in the previous design, where a StructuredMelody can hold a Melody. No more different kinds of melodies and progressions: everything is just a Sequence.

The different subclasses of MelodyGenerator and ProgressionGenerator simply implement different algorithms to produce the product Sequence.

The StructuredGenerators hold a reference to a general Generator, which means they still have the option to use another StructuredGenerator internally. Depends on them. However their default is to use a SimpleGenerator internally.


I managed to improve the design by finding the places where duplication can be eliminated by giving things a more general title: I.e. I realized Progression and Melody are practically the same thing, so I abstracted things be calling them both a Sequence.

Also, realizing the composite pattern in the melodies and progressions was unnecessary helped clear the design.

Thanks for your feedback.

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.