17

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?

deanvmc
  • 5,957
  • 6
  • 38
  • 68
  • 2
    @DeanMc: you don't need the language both in the title and in the tags. Tags is enough. – John Saunders Apr 14 '10 at 19:50
  • 1
    I'd rather see a switch statement than a ton of if then else else if then else if then else if then else if then else if then else if then else if then else if then else ... Of course, its preference... The only bad thing I see about switch statements is that you can forget a break;... of course, you could make a similar argument that in an if then else if than else, you can forget an else [forming two separate if statements] – Warty Apr 14 '10 at 19:51
  • 4
    @ItzWarty in c# you can't forget a break, missing breaks are an error. – Darryl Braaten Apr 14 '10 at 19:54
  • That is true. I actually wish they allowed you to fall through cases >_> [I <3 JavaScript] but yeah, it stops you from making errors – Warty Apr 14 '10 at 19:57
  • 1
    @ItzWarty: You can still do the same by using `goto case`. – Matti Virkkunen Apr 14 '10 at 19:58
  • 1
    @Darryl: That's not true; you can omit breaks to create fallthroughs between cases. There are some limitations on that; I believe the final case (or default) has to have a break. – Otis Apr 14 '10 at 20:01
  • Registering in a dictionary/map would be more readable. Several examples are below. – Dinah Apr 14 '10 at 20:02
  • 2
    @Otis: The only place you're allowed to omit a `break` is between two `case` s. You can't have any code between those. C# was specifically designed to prevent case fallthrough. You can explicitly specify the same functionality with `goto case`. – Matti Virkkunen Apr 14 '10 at 20:03
  • @Matti: That I did not know; I can't think of many places I've ever used a case with actual code that then fell through to more code, but it still seems harsh to not allow it. – Otis Apr 15 '10 at 01:57
  • @Otis it is only not true if there is nothing in that section. aka switch(s) { case "case1": case "case2": //Ok fall through break; case "case 3": SomeFunction();//compile error case "case4": break; } – Darryl Braaten Apr 15 '10 at 15:10

13 Answers13

23

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
  • 811,555
  • 193
  • 1,581
  • 1,452
  • Definitely like the tag/handler dictionary approach, Mark. – itsmatt Apr 14 '10 at 20:00
  • 1
    Definitely yes! Unless I am using non-object oriented language like C. – Nayan Apr 14 '10 at 21:00
  • 3
    I believe what he's referring to is essentially the [strategy pattern](http://en.wikipedia.org/wiki/Strategy_pattern). – Noldorin Apr 14 '10 at 21:11
  • 2
    @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 Apr 15 '10 at 00:44
  • @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 Apr 15 '10 at 08:26
  • @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 Apr 15 '10 at 23:42
  • @Dean: I'd call it the command pattern too. Though to some extent a comand is a specialized strategy. – Duncan Apr 15 '10 at 23:43
10

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.

Is there any significant difference between using if/else and switch-case in C#?

Community
  • 1
  • 1
kemiller2002
  • 113,795
  • 27
  • 197
  • 251
  • 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. – deanvmc Apr 14 '10 at 20:18
  • 1
    Sacrificing readability and maintainability for the miniscule cost of instantiating a small class is IMHO premature optimization. – Ian Mercer Apr 14 '10 at 21:15
  • 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. – deanvmc Apr 14 '10 at 21:36
  • @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 Apr 15 '10 at 15:48
  • Splitting it into separate classes will also make it easier to unit test. You can develop each frame type and test it in isolation. – Ian Mercer Apr 15 '10 at 16:45
5

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
  • 10,562
  • 23
  • 99
  • 147
3

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
  • 77,506
  • 18
  • 119
  • 157
2

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ș
  • 14,123
  • 2
  • 39
  • 62
  • Just remember to not go overboard. I would never use slow tagging software. – ChaosPandion Apr 14 '10 at 19:54
  • 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 – deanvmc Apr 14 '10 at 19:59
  • 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ș Apr 14 '10 at 20:02
  • It wouldn't be spaghetti, more of a funnel :) – deanvmc Apr 14 '10 at 20:13
2

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
  • 53,214
  • 7
  • 75
  • 105
  • 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 Apr 15 '10 at 00:09
1

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
  • 378
  • 2
  • 14
  • 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. – deanvmc Apr 14 '10 at 20:45
1

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.

Ian Mercer
  • 38,490
  • 8
  • 97
  • 133
  • 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. – deanvmc Apr 14 '10 at 21:16
  • It has a separate code file for each frame type making it easy to manage. – Ian Mercer Apr 14 '10 at 22:18
1

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
  • 75,627
  • 68
  • 187
  • 294
  • 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. – deanvmc Apr 15 '10 at 00:40
  • 1
    @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 Apr 15 '10 at 04:22
0

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
  • 669
  • 2
  • 10
  • 21
0

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...
deanvmc
  • 5,957
  • 6
  • 38
  • 68
0

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
  • 5,886
  • 1
  • 25
  • 41
0

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? – deanvmc Apr 15 '10 at 00:38
  • 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. –  Apr 15 '10 at 16:06