Jump to content
  • Advertisement
Sign in to follow this  
Trillian

[C#] How to refactor this?

This topic is 3608 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Hello, I've developed a library for working with concepts of music theory and I find myself having lots of duplicate code. I have four structures that are problematic : Chord, ChordType, Scale and ScaleType. The problem if that the Chord and Scale structures have implementations which are nearly the same, with minor terminology and functionality differences. Logically, chord and scales are different concepts, but their implementation is actually extremely similar. Same goes for the ChordType and ScaleType structures. That amounts for about 400 lines of duplicated code. The code "smell" (as mentionned in the book "Refactoring") is obviously Duplicated Code. But none of the refactorings seem to address a problem such as mine (two separate structs which are nearly copies). I thought of turning these 4 structures into classes and having a base class NotePattern with ChordType and ScaleType functionality as well as a NoteSet base class for Chord and Scale classes, but I'm unsure if this is the way to go. Any ideas as to how I could refactor this?

Share this post


Link to post
Share on other sites
Advertisement
Quote:
Original post by Spoonbender
Put all the common functionality into a third class that they can call into as needed?


That would allow me to migrate implementations to a common location. But actually most members have very simple implementations : most of the code is interface and documentation comments. So it wouldn't be all that beneficial. I guess the only way I can reduce interface duplication is through inheritance but I'm curious to know if there are other options available to me.

Share this post


Link to post
Share on other sites
It's a little hard to give specific advice without knowing more about how you intend to use these structs/classes, but I'll go ahead and offer a few comments.

It seems that through inheritance and/or data-driven code, you should be able to eliminate most of the duplication you mention.

I'm assuming that (for example) the ScaleType type represents types of scales such as Dorian, Mixolydian, Octatonic, and so on, while the Scale type represents a specific scale (e.g. 'F Dorian').

The first thing that comes to mind is that a 'scale' object is basically a 'scale type' object with a specified root. Again, I don't know how you're using these classes, but it makes sense that you might be able to have a scale play itself (via MIDI or whatever), an operation that wouldn't make much sense for a scale type. There are other operations as well (for example, naming the 3rd note of the scale) that make sense for a scale but not a scale type. It follows (I think) that Scale could inherit from ScaleType, with the addition a 'root' member field and various functions such as Play(), GetNthNote(), and so on.

As you point out, scales and chords are closely related. (Also, 'chord types' and 'chords' would have the same kind of relationship as scale types and scales.) Each will consist of a root and some number of notes, each of which is a certain interval above the root. The Chord class will have somewhat different behaviors than the Scale class (for example, the 'play' function would most likely play the notes together rather than in sequence - although, you might have a 'play arpeggiated' option as well), but will also have behaviors in common (such as querying for the n'th note).

I'll leave it at that for now, but maybe this will help get you pointed in the right direction. Although I know inheritance has fallen out of favor a bit as a means of facilitating code re-use, at first look it seems like it would not be an inappropriate solution here (although I'll be interested to see of others think differently).

Share this post


Link to post
Share on other sites
jyk, thanks for providing a new solution to my problem. I didn't think of having my Scale class inherit from a ScaleType. It would surely simplify my code. But I'm not sure if, designwise, a Chord is a ChordType and a Scale is a ScaleType. My current implementation uses a "has a" relationship (A Chord has a tonic and a type), which allows for the following code, which I find quite elegant :

Scale CMajor = new Scale(Note.C, ScaleType.Major);
Chord AMinor7th = new Chord(Note.A, ChordType.MinorSeventh);


I find interesting the way you tackled my problem as you're working horizontally (Chord with ChordType, Scale with ScaleType), whereas I was trying to solve it vertically (Chord with Scale, ChordType with ScaleType).

You've given me some material to think about. But if anyone has other takes on the problem, I'd be happy to hear it.

BTW all of your assumptions about my code are right. I didn't go into much detail because this isn't a question on music theory, and I don't know if there are many GDNetters familiar with it. But yes, a Chord is a ChordType with a tonic and a Scale is a ScaleType with a root.

Share this post


Link to post
Share on other sites
Quote:
It would surely simplify my code. But I'm not sure if, designwise, a Chord is a ChordType and a Scale is a ScaleType.
You may be right that it makes more sense to say that a scale has a type than to say it is a type. However, I think there's plenty of opportunity for refactoring regardless of whether you make use of inheritance or composition.

One thing I would recommend is making the code data-driven. It may be already, but the sample you posted suggests that the scale types might be enumerants (is that correct?).

A better solution, I think, would be to load all scale and chord types from a file, and then refer to them by name, e.g.:
Scale cMajor = new Scale("c", "major");
I don't know how your scale and chord data is stored currently, but I'd certainly recommend against hard-coding it.

For something like this, it may be that arriving at an optimal solution will require some trial and error. However, it seems to me that it would make sense to store a database or collection of scale types (with the data loaded from a file), and then have each scale object store a reference to a scale type object (i.e. application of the 'flyweight' pattern).

Share this post


Link to post
Share on other sites
Quote:
One thing I would recommend is making the code data-driven. It may be already, but the sample you posted suggests that the scale types might be enumerants (is that correct?).


Nope, my ScaleType structure has a single immutable member which is an int[] of note offsets. The constructor illustrates this :
new ScaleType(0, 2, 4, 5, 7, 9, 11); // Major scale

So ScaleType.Major actually refers to :
public static readonly ScaleType Major = new ScaleType(0, 2, 4, 5, 7, 9, 11);


Quote:
A better solution, I think, would be to load all scale and chord types from a file, and then refer to them by name, e.g.:
Scale cMajor = new Scale("c", "major");
I don't know how your scale and chord data is stored currently, but I'd certainly recommend against hard-coding it.


This code is used often in small projects and prototyping speed is what I focus on. I don't really see having my static members as "hard-coded", I see them as hum... "ready-to-use presets". They can be ignored as needed. Your idea of loading it from a file will become useful if any of my projects reach a reasonable scale. Adding serialization functionality would be a good move.

One thing I didn't mention is that those classes are designed to be used by AIs more than by humans. As such, it is more important for them to be easily constructable from a set of values and easily used than to be referencable by a name.

But we're drifting a bit from my original code duplication problem (even if I appreciate your insights). I think that writing a common base class to ChordType and ScaleType and to Chord and Scale might be my only solution to remove duplicated code. Or I could just have one class that can be used interchangeably for chords and scales and one for chord types and scale type... looks like I haven't found the perfect solution yet!

BTW thanks a lot for your replies, I appreciate it. I'd raise your rating if I could still do it =).

Share this post


Link to post
Share on other sites
What can a ChordType do? A ScaleType?

Aside from "a Chord should have a ChordType and a Scale should have ScaleType, and not a cross-combination of those", is there any logical difference between the two? Is it really necessary to introduce a type-distinction there, or can you catch any relevant errors at runtime?

Can you generalize terminology?

What I'm getting at: have a Chord and a Scale (which possibly implement interfaces, derive a common base, or have one derived from the other - whatever minimizes the duplication and is logical), each of which has-a PitchOffsetSequence. :) Make sure there is no Chord-specific or Scale-specific functionality in the PitchOffsetSequence (but you should be able to get away with things like, e.g. .playAllSimultaneously(int lowestPitch)). If you must have the compile-time checks, you can derive ChordPitchOffsetSequence and ScalePitchOffsetSequence, which add nothing new, and modify the Chord and Scale interfaces accordingly; but that will inhibit reuse.

Unless, of course, you can use parametric polymorphism (e.g. C# generics). :)

(In something like Python, however, this amount of structured-ness is massive overkill, because you can't get compile-time type checks *anyway*.)

Share this post


Link to post
Share on other sites
Hey Zahlman, thanks for our input!

I think you've provided the best solution to my problem yet. ChordType and ScaleType classes are indeed virtually identical and could be merged without problem. Yep, that's a great start in reducing code duplication.

As for chords and Chords and Scales, after some thought, I've realized that they are two different ways of working with the exact same set of data : a series of notes that can exist on any octave. With this perspective in mind, it would be logical for them to have either a common base class, or to once again "generalize terminology" and merge these two as one. I'm thinking of one of the following designs :

// 1. ChordType/ScaleType as NoteOffsetSequence, Chord/Scale as NoteSet.
NoteOffsetSequence { int[] offsets }
NoteSet { Note root, NoteOffsetSequence offsets }

// 2. ChordType/ScaleType as NoteOffsetSequence, Chord/Scale have a NoteSet.
NoteOffsetSequence { int[] offsets }
NoteSet { Note root, NoteOffsetSequence offsets }
Chord { NoteSet notes }
Scale { NoteSet notes }

// 3. ChordType/ScaleType as NoteOffsetSequence, Chord/Scale are a NoteSet.
NoteOffsetSequence { int[] offsets }
NoteSet { Note root, NoteOffsetSequence offsets }
Chord : NoteSet {}
Scale : NoteSet {}


While approaches 2 and 3 might be more accurate logically, design #1 allows using chord as scales without problem, and that can be cool for things like arpeggios.

Quote:
Unless, of course, you can use parametric polymorphism (e.g. C# generics). :)


Did you say that with any ideas in mind? I'd be curious to know how you'd use generics here.

I'll start with the NoteOffsetSequence refactoring right away and see how it goes.

Share this post


Link to post
Share on other sites
Quote:
Hey Zahlman, thanks for our input!

I think you've provided the best solution to my problem yet. ChordType and ScaleType classes are indeed virtually identical and could be merged without problem.
I mention this only to salvage my ego a bit - :) - but I actually made some mention of these similarities in an earlier post:
Quote:
As you point out, scales and chords are closely related...Each will consist of a root and some number of notes, each of which is a certain interval above the root. The Chord class will have somewhat different behaviors than the Scale class...but will also have behaviors in common (such as querying for the n'th note). [Emphasis added, of course.]
I was trying to lead you towards having these two classes share code (e.g. via inheritance), but I suppose I should have been more specific. You got there eventually though (with Zahlman's help), and that's the important thing :)

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

Participate in the game development conversation and more when you create an account on GameDev.net!

Sign me up!