Gotophobia in switch case

Question

I have next code:

switch (curState.ToString())
{

   case "Processed":
         ... *(couple code lines)*
         break;
    case "NotPresent":
         if (someValue == null)
         {
              goto case "Undefined";
         }
         goto case "Processed";
   case "Undefined":
        break;
}

Someone told me that it is better to define method for "NotPresent" case and call it instead of goto case "Processed". Is this Gotophobia or reasonable? I like my variant.

There are many resources out there already explaining when and when not to use `goto` statements. Here is a great example : http://stackoverflow.com/a/11906082/3922214 — Jonathan Carroll, May 16 '16 at 19:18
If you don't want to encapsulate the details in a separate method, you can always go with setting boolean flags and handle the processing after the switch statement. I honestly think that `goto` should've never been included in C#, and it should've been deprecated and then phased out for VB.NET. The main problem I have with `goto` is it causes leap-frogging. There isn't a flow to it. — Chris Fannin, May 16 '16 at 19:26
Did you intend the `default` case to just do nothing? I know opinions on this kind of thing are subjective, but I find the code you posted here to be hard to read. — Matthew Watson, May 16 '16 at 19:28
Is that your actual code? or is your code more complicated than that? — Sam I am says Reinstate Monica, May 16 '16 at 19:30
To be honest, this question would be better suited on http://codereview.stackexchange.com/ - answers are going to be subjective and therefore this question is not a good fit for this site. — Matthew Watson, May 16 '16 at 19:41
@MatthewWatson except as-is, this question can be expected to be closed as *hypothetical/example code* within minutes if not seconds. — Mathieu Guindon, May 16 '16 at 19:50
@Mat'sMug True, http://programmers.stackexchange.com/ would probably be better. But it doesn't belong here for sure. — Matthew Watson, May 16 '16 at 20:05
@MatthewWatson not even. It's just a poor question, phrased in an opinion-seeking way that's off-topic just about everywhere on the network. It *would* be on-topic on [codereview.se] **if** OP included their *actual, real code*, some surrounding context, and explanation of what problem the code is solving, and interest in feedback on **any & all facets of the code** rather than merely being focused on *gotophobia* (although *gotophobia* **is** a valid concern to express in a good CR post). — Mathieu Guindon, May 16 '16 at 20:10

Jonesopolis · Answer 1 · 2016-05-16T19:31:33.800

3

Any answer is speculation and opinion. But I believe it's general opinion (and written in several places on the internet) that the 2 acceptable uses of goto are:

Breaking out of nested loops
Falling through one switch case to another, as C# doesn't allow a case with any code in it to fall through to the next condition.

In your case, you are actually applying logic and jumping around based on a condition. To me, that isn't an acceptable use for goto, and should be refactored.

Of course, your code works, and you are aware of the negative connotation around goto. So whatever you decide will be a well informed decision. I'd say make a decision and don't worry about it until a need arises.

edited May 16 '16 at 19:31

answered May 16 '16 at 19:28

Jonesopolis

25,034
12
68
112

Nice answer, but point #2 isn't entirely accurate, if a case has no code then fall throughs are fine. ie `case state1: case state2:` – May 16 '16 at 19:30
good point, i'll clarify. – Jonesopolis May 16 '16 at 19:31
I like very much your last paragraph thought. But first one is like: "That clevel guys told that goto is appropriate only in two cases". I Got specification that describes some format internal states and I am going from one to other in the order of use. specification say if someValue is null do like in Undefined case if nt null like in Processed case. That is VERY clear.. I don't need magic with falgs or reodering that mess up possible states order. Why don't I care what someone told abut goto if here it is clear and easy to undestand. – Brans Ds May 16 '16 at 21:02
@BransDs I tried my best to preface that this is opinion. I'd be hesitant to tell anyone to use goto in the above cases, because I'd be nervous that they'd get too comfortable and find another place to use it. I trust myself though. So, do what you think is right. – Jonesopolis May 16 '16 at 21:04

score 0 · Answer 2 · answered May 16 '16 at 19:36

I like my variant.

I dislike your variant. Gotos are appropriate when using them allows you to write clearer code than you can write without. I don't think that criterion has been satisfied in this case.

Someone told me that it is better to define method for "NotPresent" case and call it instead of goto case "Processed". Is this Gotophobia or reasonable?

Do you mean you were told it would be better to define a method for the body of the "Processed" case, to be invoked unconditionally in that case and conditionally in the "Not Present" case, thereby avoiding one goto? I don't think that would help matters much. As long as the "Processed" case body contains just a few statements, I find it clearer to keep those in the body of the switch.

Myself, I would probably rewrite your switch like so:

switch (curState.ToString())
{
    case "NotPresent":
         if (someValue != null)
         {
             goto case "Processed";
         }
         break;
    case "Processed":
         ... *(couple code lines)*
         break;
}

There's no point in branching to a case that does nothing -- as opposed to simply doing nothing in the first place -- so I removed that goto altogether. I moved the "NotPresent" case above "Present" because that makes the pair reminiscent of a single switch section with two case labels.

I also removed the empty case for "Undefined", but if it were important to emphasize that no other values of curState.ToString() were expected, then I would instead retain that case and add a default case with appropriate assertion / throw / logging.

score 0 · Answer 3 · answered May 16 '16 at 19:51

Instead of trying to force your logic into a switch statement, why not use a simpler approach, since you do nothing with your undefined state:

if ((curState.ToString() == "Processed") ||
   ((curState.ToString() == "NotPresent") && someValue != null))
{
    ... *(couple code lines)*
}

Gotophobia in switch case

3 Answers3