tags:

views:

819

answers:

13

I am in the final stages of creating an MP4 tag parser in .Net. For those who have experience with tagging music you would be aware that there are an average of 30 or so tags. If tested out different types of loops and it seems that a switch statement with Const values seems to be the way to go with regard to catching the tags in binary.

The switch allows me to search the binary without the need to know which order the tags are stored or if there are some not present but I wonder if anyone would be against using a switch statement for so many conditionals.

Any insight is much appreciated.

EDIT: One think I should add now that where discussing this is that the function is recursive, should I pull out this conditional and pass the data of to a method I can kill?

+10  A: 

Personally, if you must, I would go this way. A switch statement is much easier to read than If/Else statements (and at your size will be optimized for you).

Here is a related question. Note that the accepted answer is the incorrect one.

http://stackoverflow.com/questions/395618/if-else-vs-switch/395965#395965

Kevin
Cheers for the link, reading now.
DeanMc
In the end I think I will got with a vanilla switch. While all of the other answers have there merits (and upped accordingly) I need to be mindful of performance, every extra piece of infrastructure I add detracts from closing the file in use which is a bad thing. The code is fairly lean as is so this wont kill readability.
DeanMc
Sacrificing readability and maintainability for the miniscule cost of instantiating a small class is IMHO premature optimization.
Hightechrider
I would argue that mine is easier to maintain give that it is only as developed as much as needed. It's clean to read an lives in one place in the code with no dependancy code.
DeanMc
@Hightechrider, @DeanMc - This is not exactly some constantly changing standard which will need to be rewritten every year. I would prefer simple code in this case.
ChaosPandion
Splitting it into separate classes will also make it easier to unit test. You can develop each frame type and test it in isolation.
Hightechrider
+3  A: 

For something low level like this I don't see a problem. Just make sure you place each case in a separate method. You will thank yourself later.

ChaosPandion
+1 for putting the body in separate functions.
BCS
+2  A: 

To me, having so many conditions in a switch statement gives me reason for thought. It might be better to refactor the code and rely on virtual methods, association between tags and methods, or any other mechanism to avoid spagetti code.

Cătălin Pitiș
Just remember to not go overboard. I would never use slow tagging software.
ChaosPandion
This is why I thought about a switch statement. The amount of tags is indeed large but finite and I am worried that the overhead of creating more objects on the stack/heap would reduce performance
DeanMc
I can't assess the performance impact on C#, I don't have enough experience with that. It might be acceptable, for a finite set of tags, to have spagetti code...
Cătălin Pitiș
It wouldn't be spaghetti, more of a funnel :)
DeanMc
+5  A: 

Another option (Python inspired) is a dictionary that maps a tag to a lambda function, or an event, or something like that. It would require some re-architecture.

Hamish Grubijan
+1 Mapping to delegates or functions would make for manageable and readable code
Dinah
I've never worked with lambas but I will have a read to see what its all about
DeanMc
Actually Lambdas are meant to be short-lived. A delegate seems most appropriate here.
Hamish Grubijan
@Hamish - Lambdas are simply syntactic sugar for delegates.
ChaosPandion
+22  A: 

It'll probably work fine with the switch, but I think your function will become very long.

One way you could solve this is to create a handler class for each tag type and then register each handler with the corresponding tag in a dictionary. When you need to parse a tag, you can look up in the dictionary which handler should be used.

Mark Byers
totally agreed.
Definitely like the tag/handler dictionary approach, Mark.
itsmatt
Definitely yes! Unless I am using non-object oriented language like C.
Nayan
I believe what he's referring to is essentially the [strategy pattern](http://en.wikipedia.org/wiki/Strategy_pattern).
Noldorin
@Nayan: There's nothing stopping you from registering function pointers against strings in vanilla C. I can't think of anything an OOP language offers that can't be replicated.
Duncan
@Duncan: I think most of the languages have strong capabilities. But the one which offers "easiest" and robust features to handle the situation will be preferred. I have nothing against C (I still like it) but I feel that using OOP features and as Noldorin said, applying patterns will make the code easier and cleaner. :)
Nayan
I would have called it the command pattern.
Dean J
@Nayan: Agreed. I was pointing out that you *can* do it in C, not that you'd *want* to :). Though I think you would still want to apply the patterns if you were forced to work in C. If it's difficult to do anything you might as well have difficulty doing it correctly.
Duncan
@Dean: I'd call it the command pattern too. Though to some extent a comand is a specialized strategy.
Duncan
A: 

I'm not familiar with the MP4 technology but I would explore the possiblity of using some interfaces here. Pass in an object, try to cast to the interface.

public void SomeMethod(object obj)
{
  ITag it = obj as ITag;

  if(it != null)
  {
    it.SomeProperty = "SomeValue";
    it.DoSomthingWithTag();
  }
}
Chris L
I see where you are coming from but the overhead is too much, thanks for the response though.
DeanMc
+2  A: 

If you have only one place that has that particular structure of switch and case statements, then it's a matter of style. If you have more than one place that has the same structure, you might want to rethink how you do it to minimize maintenance headaches.

MSN
It is only in one place.
DeanMc
Even if you only have it in one place (for now), you might consider building it so that the cases are enumerator values. That way if you make another one later, the compiler will force you to include all the different tags (or a default) even if it is just to ignore them. In the same way, that will keep things aligned when you add a tag to one of the switches.
BCS
+1  A: 

It's hard to tell without seeing your code, but if you're just trying to catch each tag, you could define an array of acceptable tags, then loop through the file, checking to see if each tag is in the array.

Jim Greenleaf
The code is very hairy at the moment so it will be a while before I release the source. But I see what your saying.
DeanMc
A: 

I wanted to add my own answer just to bounce off people...

  1. Create an object that holds the "binary tag name", "Data", "Property Name".
  2. Create a list of these totaling the amount of tags known adding the tag name and property name.
  3. When parsing use linq to match the found name with the object.binarytagname and add the data
  4. reflect into the property and add the data...
DeanMc
A: 

What about good old for loop? I think you can design it that way. Isn't switch-case only transformed if-else anyway? I always try to write code using loop if amount of case statements is becoming higher than acceptable. And 30 cases in switch is too high for me.

Tomas Voracek
+1  A: 

ID3Sharp on Sourceforge has a http://sourceforge.net/projects/id3sharp/ uses a more object oriented approach with a FrameRegistry that hands out derived classes for each frame type.

It's fast, it works well, and it's easy to maintain. The 'overhead' of creating a small class object in C# is negligible compared to opening an MP4 file to read the header.

Hightechrider
I must take a look at that, sounds like and awful lot of infrastructure when and a small method can just assign them to actual properties of a "file" class.
DeanMc
It has a separate code file for each frame type making it easy to manage.
Hightechrider
+1  A: 

One design that might be useful in some case (but from what I've seen would be over kill here):

class DoStuff
{
   public void Do(type it, Context context )
   {
      switch(it)
      {
          case case1: doCase1(context) break;
          case case2: doCase2(context) break;
          //...
      }
   }
   protected abstract void doCase1(Context context);
   protected abstract void doCase2(Context context);
   //...
}

class DoRealStuff : DoStuff
{
   override void doCase1(Context context) { ... }
   override void doCase2(Context context) { ... }
   //...
}
BCS
I see what your doing there but im unsure of the value of the extra wiring if at the end of it all I still have the switch statement.
DeanMc
@DeanMc: As far as a dispatch device goes, I don't think you can get better than a switch statement (as long as you have a static set of options). The potential advantage is two part in that you can 1) encapsulate and reuse the dispatch and 2) you get a compile error if you do it wrong, e.i. you attempt to reuse it an forget to deal with a case or you if add a case in one place you can't forget to add it everywhere else as well.
BCS
A: 

You almost certainly have a Chain of Responsibility in your problem. Refactor.

I don't follow given that it is one point. How would you refactor out this without introducing code that increase the amount of objects live during runtime for future possibilities that may never happen?
DeanMc
I would never write anything for future possibilities that may never happen. A switch statement with thirty conditions represents current actualities that have already happened. I don't get the rest of your comment.