Question

This seems to come up alot in my code, i'm wondering if there is some way of removing the switch statement, or if there is a more elegant way of doing it?

    public class MetaData
{
    public string AlbumArtist { get; set; }
    public string AlbumTitle { get; set; }
    public string Year { get; set; }
    public string SongTitle { get; set; }


    public static MetaData CreateMetaDataFrom(IEnumerable<TextFrame> textFrames)
    {
        var metaData = new MetaData();

        foreach (var frame in textFrames)
        {
            switch (frame.Descriptor.ID)
            {
                case "TPE1":
                    metaData.AlbumArtist = frame.Content;
                    break;

                case "TALB":
                    metaData.AlbumTitle = frame.Content;
                    break;

                case "TIT2":
                    metaData.SongTitle = frame.Content;
                    break;

                case "TYER":
                    metaData.Year = frame.Content;
                    break;
            }
        }

        return metaData;
    }
}
Was it helpful?

Solution

From your code I conclude that IEnumerable<TextFrame> has always 4 members so you could just write (haven't tried it so check for the syntax):

public static MetaData CreateMetaDataFrom(IEnumerable<TextFrame> textFrames)
{
    return new MetaData()
    {
        metaData.AlbumArtist = textFrames.Where(frame => frame.Descriptor.ID = "TPE1").SingleOrDefault().Content,
        metaData.AlbumTitle = textFrames.Where(frame => frame.Descriptor.ID = "TALB").SingleOrDefault().Content, 
        metaData.SongTitle = textFrames.Where(frame => frame.Descriptor.ID = "TIT2").SingleOrDefault().Content;
        metaData.Year = textFrames.Where(frame => frame.Descriptor.ID = "TYER").SingleOrDefault().Content;
    };
}

OTHER TIPS

This is related to an object-oriented approach. The usual method to get rid of if's or case's is to use a lookup table of criteria and effects. There are other techniques that use this same idea, such as data-directed programming (http://en.wikipedia.org/wiki/Data-directed_programming) and dispatch tables (http://en.wikipedia.org/wiki/Dispatch_table). Many language implementations use type dispatch tables to implement virtual method calls.

The lookup table could be a hashtable populated with lambda functions as so:

Dictionary<string, Func<MetaData, string, string>> lookup = new Dictionary<string, Func<MetaData, string, string>>();
lookup["TPE1"] = (m, v) => m.AlbumArtist = v;
lookup["TALB"] = (m, v) => m.AlbumTitle = v;
lookup["TIT2"] = (m, v) => m.SongTitle = v;
lookup["TYER"] = (m, v) => m.Year = v;

then you assign fields of metaData inside your loop as:

lookup[frame.Descriptor.ID](metaData, frame.Content);

You might want to look at implementing the Strategy Pattern. DimeCasts.Net have an excellent video tutorial that may help.

I was tempted to suggest the Strategy Pattern but you may need a slight variation. Consider writing a method in the TextFrame class, lets call it putContent(MetaData).

Then create subclasses of TextFrame each representing a different Type of Frame. Each subclass will override the putContent(Metadata) method and do its appropraite logic.

PseudoCode Example for TPE1:

 Metadata putContent(MetaData md){
       md.AlbumArtist = Content;
       return md;
 }

You MetaData code will then change to:

var metaData = new MetaData();

    foreach (var frame in textFrames)
    {
           metaData = frame.putContent(metaData);
    }
 return metaData;

Of course, to create the TextFrames them selves will then require a Factory so this is not the end of the story.

It seems like you know what the types will be before hand (using a switch) so why not just retrieve the values as required, without a for switch.

Examples are when using a hashtable, and you know what fields will be available, just use the fields.

ig you are not sure if the field is available, a simple test will be sufficiant if the list contains the value.

You can then even write a helper function to check and return the value if the list has the value.

What you really have here is a four-way setter. The canonical refactoring here is "Replace Parameter with Explicit Methods" (p285 of Refactoring by Martin Fowler). The Java example he gives is changing:

void setValue(String name, int value) {
  if (name.equals("height")) {
    _height = value;
    return;
  }
  if (name.equals("width")) {
    _width = value;
    return;
  }
}

to:

void setHeight(int arg) {
  _height = arg;
}

void setWidth(int arg) {
  _width = arg;
}

Assuming that the caller of CreateMetaDataFrom() knows what it's passing in, you could skip the switch/case and use the actual setters for those properties.

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top