6

This thread will be long, but I will try to make it as short as I can. Thank you.

I have recently implemented a relatively simple program. What this program does is generate a simple piece of music and play it.

I will describe the object oriented design of the program and would like to hear your review of the design. Especially how well it fits the SOLID principles, where it doesn't, and how I can improve my design to be more SOLID in the future.

(If there's something in the description that isn't clear enough, please tell me.)

I have divided my design into two parts.

1- The "behind the scenes" system that generates the content - a series of notes and chords (as will be explained later) - and is responsible for playing it.

2- The system that provides a UI to control the "behind the scenes" system. This system tells the other system when to create new music and when to play it.

I will describe each system separately.


System 1 - responsible for the generating and playing music.

The way music is created is by first generating a chord progression (a series of chords to be played one after the other), and then generating a melody based on that progression. I will describe how this is designed 'from the bottom up'.

Generating a chord progression.

Class Note: the most basic class. Responsible for playing sound files, and is set in instantiation to play a specific sound (C, D, etc.). It has an interface to play() and to stop() the playback. It also has a method getName() to identify it (the name is fed to the Note through the constructor).

Class Chord: this class is instantiated with an array of three Note objects and a name. For example, a Chord named cMajor will be instantiated with the Notes c, e, and g and the string "CMajor". Chord has an interface to play() and stop() the notes it's composed with, has a method getName() and a method getNotes() that returns the array of Notes.

Class Progression: this class is instantiated with an array of Chord objects, and has logic to play and stop them one after the other in a particular tempo (speed, in BPM - beats per minute). It has a method play(int bpm) and a method getChords() that returns the array of Chords.

Class ProgressionGenerator: implements an algorithm to create a particular series of Chord objects, and instantiate a Progression with these chords.

To summarize: a Progression is composed of Chords, which are composed of Notes. The ProgressionGenerator instantiates a Progression with a particular series of Chords.

Generating a melody.

Class MelodyNote: composed with a single Note. It has an attribute double duration which specifies for how long this note is to be played relative to a measure/bar of music (e.g. 'half note' is specified as 0.5, 'quarter' as 0.25). It has a method getDuration() and an interface to play() and stop() it's inner Note (basically delegates to the Note).

Class Melody: composed with a series of MelodyNotes. Has logic to play the series of notes in a particular speed. It uses each MelodyNote's duration to know when to stop a note and play the next one.

Class MelodyGenerator: implements an algorithm to generate a series of MelodyNotes and instantiate a Melody with it. It does this based on a Progression. The method signature is Melody generate(Progression prog).

So to summarize:

Progression --> composed with Chords --> composed with Notes

Melody --> composed with MelodyNote

Each object controls it's inner objects and exposes them. For example Chord has logic to play it's inner Notes, and has a method getNotes() that returns them to whoever needs to know. For example, MelodyGenerator uses progression.getChords() in order to build a melody according a particular series of chords. It also uses chord.getNotes() when choosing what notes to place over a chord.

One last thing: the generators use the singleton class NotesAndChordsSupplier to get the Note and Chord instances they need. It hands them existing instances.


System 2 - responsible for providing a UI to control the creation and playback of music.

This system is designed using the MVC pattern. The Model encapsulates the creation and playing of music. The View is the UI that provides buttons to tell the Model to play and make new music. The Controller is notified by the View when buttons are pressed and invokes the proper actions on the Model.

The Model

The Model contains a Progression, a ProgressionGenerator, a Melody and a MelodyGenerator. When it's told to generate new music, it simply delegates the task to the generators. When it's told to play music, it delegates the task to the melody and progression. Simplified code:

public void makeNewMusic(){
    progression = progGenerator.generate();
    melody = melodyGenerator.generate(progression);
}

public void playMusic(){ // this actually starts two separate threads, but irrelevant.
    progression.play();
    melody.play();
}

The Model is connected (in a loosely-coupled manner) to two objects: it's Melody and the View - both via the Observer pattern.

The Model implements two interfaces: Observer and Observable. It's registered as an observer to the Melody, and the View is registered as an Observer to the Model.

When the Melody finishes it's playback, it notifies it's observer - the Model. When the Model is notified, it notifies it's own observer - the View.

This way, the View gets notifies when playback is finished, so it can re-enable it's UI buttons that were disabled during playback.

The View

The View is the UI and it has two buttons: Play and New Tune. They invoke playButtonPressed() and newTuneButtonPressed() on the Controller, respectively.

The View also features enableButtons() and disableButtons() methods.

As I said, the View is registered as an Observer to the Model. It is notified when playback of the music is finished, so it can re-enable it's buttons that were disabled by the Controller when playback started.

The Controller

When the View calls newTuneButtonPressed() on the Controller, it simply delegates to the Model: model.makeNewMusic().

When the View invokes playButtonPreesed() on the Controller, two things happen. 1- The Controller calls disableButtons() on the View. 2- The Controller calls playMusic() on the Model.

All buttons on the View are disabled by the Controller when music playback starts.

The Controller registers the View as an observer to the Model, and the Model as an observer to it's member Melody.


UML class diagram to illustrate the architecture

(Please note: in this post I ommited a class Scale, which isn't important. Ignore it in the diagram).

enter image description here


Thank you for reading all of this. I will appreciate any kind of criticism of the object oriented design of the program.

Especially, how well are SOLID and OO principles integrated in my design, and how can I design more in the spirit of SOLID on the future.

closed as too broad by gnat, amon, Dynamic, JBRWilkinson, Jörg W Mittag May 12 '14 at 1:00

Please edit the question to limit it to a specific problem with enough detail to identify an adequate answer. Avoid asking multiple distinct questions at once. See the How to Ask page for help clarifying this question. If this question can be reworded to fit the rules in the help center, please edit the question.

  • I've removed your link to the full size picture, which Websense identified as "adult/swimwear apparel." If anyone wants to see the full size graphic, they can just right-click the picture and open it in a new tab or window. – Robert Harvey Apr 29 '14 at 23:59
  • This should go on codereview.stackexchange.com – Idan Arye Apr 30 '14 at 0:26
  • 4
    @IdanArye: Code Review only accepts code from functioning programs for review, not designs. – Robert Harvey Apr 30 '14 at 0:28
  • Be careful about embedding the names into your objects. Identical notes have different names depending on the key. You can instead use a number that can be used to identify the octave and the note (The MIDI value can be good for this). – DanDan May 1 '14 at 12:23
  • Surely you are missing an instrument for playing melodies. Note shouldn't be playing itself. – CodeART May 1 '14 at 15:51
7

I'm not sure I can give you a thorough review via internet, but some notes as I read through:

  • There is more than one note with the same name. Low C, middle C, high C, etc. And even there, "high"/"low" is relative to the instrument playing them, so those probably aren't sufficient names.
  • I don't particularly like Note knowing how to play itself. The same note can be played by different instruments, for different durations, with different techniques (trill, staccato, etc.).
  • I don't particularly like having a play and stop. This adds temporal coupling (you can only stop after play. Play with a duration may be better.
  • I don't particularly like Note being constructable. There is a well defined set of notes. Making them static and immutable makes them more easily comparable, better in concurrent scenarios, easier to reason about...
  • You don't mention the language, but getBlah methods are smelly to me.
  • Progression has a beats per minute, but that assumes each chord gets equal time. It also assumes that there's no transition between chords/notes (slurs, percussion effects).
  • There's no reason to have a singleton. Even if you don't want the notes and stuff to be static & immutable, having more than one flyweight isn't going to end the world. If you only want one, only make one.
  • As a matter of practicality, you will likely need to test playing the notes in sequence (if that is what you plan to do) since program (and sound card) latency may make it sound bad.
  • I am not sure your generators are sufficiently generic enough to include directly in the model. You should consider having interfaces for those dependencies.
  • Your Observer and Observable interfaces are weird. Even in a language without delegates/events, you'd still give these things concrete names or some message to pass in the function call. These make it seem as though there's only ever one (implied) event signaled between two objects.

...and that's about it. Hope that helps.

  • Thanks for answering. A number of questions: 1- Why do you think it's a bad idea to have a singleton that stores all the notes and chords? 2- The way I designed it, is that the Melody takes care of when to play and stop notes, instead of the note taking care of it by itself (i.e. stopping itself after it's duration). Why do you think it would be better for the Note to do it itself, instead of the Melody managing all of this 'from above'? 3- Do you mean that I should create interfaces for the generators, and have the Model depends on them, instead of the concrete generators? – Aviv Cohn Apr 30 '14 at 11:07
  • @prog - A singleton is an anti-pattern. It forces you to only ever have one of an object, and that's not a requirement here. I don't think Note should play itself (point 2). Instead of your model taking the actual ProgressionGenerator (For example), it may be better to take IProgressionGenerator to allow different algorithms when that need inevitably arises – Telastyn Apr 30 '14 at 12:00
  • I see. How can I tell when an object should depend on an interface and when it should depend on a concrete type? I understand the flexibility with depending on interfaces, but how can I sense when an object should depend on an interface and when it can depend on an actual class? – Aviv Cohn Apr 30 '14 at 12:25
  • 2
    @prog - yes, that is pretty much what they mean. You wouldn't want to prevent yourself from having dudes with chainsaws on their hands after all. – Telastyn Apr 30 '14 at 14:24
  • 1
    @prog - that is probably too long/complicated for me to answer here. – Telastyn Apr 30 '14 at 17:10

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