CODEWITHSUNDEEP
c#oopdesign-patterns
Totem
Totem

Reputation: 474

How to write a clear and elegant code for a strategy pattern based application?

I am writing an application to compose musical freizes, but I am a little worried about the structure of classes and interfaces within the code. Here there are the signatures of some classes I wrote:

Interface IGuidoFormattable
Class Note : IGuidoFormattable, ICloneable
Class Pause : IGuidoFormattable, ICloneable
Class Chord : List<Note>, IGuidoFormattable, ICloneable
Class Key : IGuidoFormattable, ICloneable
Class Tempo : IGuidoFormattable, ICloneable
Class Meter : IGuidoFormattable, ICloneable
Class FretPiece : List<IGuidoFormattable>, ICloneable
Class Fret : List<FretPiece>

FretPiece represents a muscial phrase, a piece of the complete freize. It exposes as properties Key, Tempo and Meter, which are homonym with their types. More phrases put together create a freize, respresented by the Fret class. Every element within a single phrase must be formattable in accordance with the GUIDO standard notation, hence it has to implement the IGuidoFormattable interface. In another namespace, some mutation classes are defined and they all inherit from one of the two abstract classes:

Class FretMutation
Class LambdaFretMutation : FretMutation

Finally, there exists a class called FretMutationGenerator which has the only task of applying selected mutations to a music theme and output the entire freize as an instance of Fret class.

FretPiece must be able to contain several different elements (notes, pauses and chords in this case), which nonetheless must satisfy two constraints: they have to be formattable with GUIDO notation and therefore transformed into meaningful strings; they have to be cloneable. In the code as it is now, every class implements ICloneable, but syntax and semantics of current code do not grant that all members of the collection are cloneable. I need to find a way to express both constraint without applying inheritance to IGuidoFormattable and preferably without defining a Clone method in the IGuidoFormattable interface.

Second, and most important, problem. FretMutation defines an abstract method, "Apply", that has to be overridden in every derived class. Therefore, any mutation class defines its own version of this method, which has the following signature:

FretPiece Apply(FretPiece originalTheme)

It accepts as input a FretPiece and outputs a copy of that object, mutated according to all other parameters specified as members of the class. I think that this is an implementation of the strategy pattern. However, only becase of the fact that this method creats a copy of the input, it means that the argument itself (and therefore all its members) must be cloneable. In addition, FretPiece is declared as a list of IGuidoFormattable, but every mutation class behaves differently from the others and may act on notes, pauses or chord, accordingly: this means that I need to check every element's type, and write a different code for each type with "a lot" (indeed, 3 at most) if statements. And this seems to me very little object oriented.

How can I arrange classes and interface in a way that all become more object oriented and less dependent on assumptions and type-checking?

Upvotes: 3

Views: 423

Answers (2)

flq
flq

Reputation: 22849

I need to find a way to express both constraint without applying inheritance to IGuidoFormattable and preferably without defining a Clone method in the IGuidoFormattable interface

What about the third option?

public interface ICloneableAndGuidoFormattable : IGuidoFormattable, ICloneable { }

Then your FretPiece is List Of ICloneableAndGuidoFormattable

If not that, you could try such a construct:

public interface ICloneable<T>
{
  T Clone();
}

public class FretPiece : IEnumerable<IFormattable>, ICloneable<FretPiece>
{
    private List<IFormattable> items = new List<IFormattable>();

    public void Add<T>(T value) where T : IFormattable, ICloneable<IFormattable>
    {
        items.Add(value);
    }

    public IEnumerator<IFormattable> GetEnumerator()
    {
        items.GetEnumerator();
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }

    public FretPiece Clone()
    {
        return new FretPiece { items = new List<IFormattable>(
            items.Cast<ICloneable<IFormattable>>().Select(c=>c.Clone()))
        };
    }
}

And somewhere else e.g. on your mutator:

public T Apply<T>(T fretPiece) where T : IEnumerable<IFormattable>, ICloneable<T> ( ...)

This would ensure that you can only add items implementing both interfaces. the enumeration only assumes that IFormattables are returned. This would allow you inside the cast to safely cast to ICloneable since it must have passed the type constraint on "Add". You can see the implementation of clone. Even though you have a cast there it is safe unless somebody fiddles with items based on reflection ;)

Upvotes: 3

CodesInChaos
CodesInChaos

Reputation: 108790

I would make several of them immutable. That way you don't need to clone them anymore. A note is always the same note, regardless of where it appears. So it feels natural to make it immutable.

For example Note,Pause,Chord seem like they would work well as immutable types. Most of your other class names are gibberish to me since I don't know enough about musical theory, so I don't know which of them could be immutable too.

Upvotes: 2

Related Questions