47

I have a switch structure that has several cases to handle. The switch operates over an enum which poses the issue of duplicate code through combined values:

// All possible combinations of One - Eight.
public enum ExampleEnum {
    One,
    Two, TwoOne,
    Three, ThreeOne, ThreeTwo, ThreeOneTwo,
    Four, FourOne, FourTwo, FourThree, FourOneTwo, FourOneThree,
          FourTwoThree, FourOneTwoThree
    // ETC.
}

Currently the switch structure handles each value separately:

// All possible combinations of One - Eight.
switch (enumValue) {
    case One: DrawOne; break;
    case Two: DrawTwo; break;
    case TwoOne:
        DrawOne;
        DrawTwo;
        break;
     case Three: DrawThree; break;
     ...
}

You get the idea there. I currently have this broken down into a stacked if structure to handle combinations with a single line instead:

// All possible combinations of One - Eight.
if (One || TwoOne || ThreeOne || ThreeOneTwo)
    DrawOne;
if (Two || TwoOne || ThreeTwo || ThreeOneTwo)
    DrawTwo;
if (Three || ThreeOne || ThreeTwo || ThreeOneTwo)
    DrawThree;

This poses the issue of incredibly long logical evaluations that are confusing to read and difficult to maintain. After refactoring this out I began to think about alternatives and thought of the idea of a switch structure with fall-through between cases.

I have to use a goto in that case since C# doesn't allow fall-through. However, it does prevent the incredibly long logic chains even though it jumps around in the switch structure, and it still brings in code duplication.

switch (enumVal) {
    case ThreeOneTwo: DrawThree; goto case TwoOne;
    case ThreeTwo: DrawThree; goto case Two;
    case ThreeOne: DrawThree; goto default;
    case TwoOne: DrawTwo; goto default;
    case Two: DrawTwo; break;
    default: DrawOne; break;
}

This still isn't a clean enough solution and there is a stigma associated with the goto keyword that I would like to avoid. I'm sure there has to be a better way to clean this up.


My Question

Is there a better way to handle this specific case without effecting readability and maintainability?

29
  • 28
    it sounds like you want to have a big debate. But you'll need a better example of a good case to use goto. flag enum neatly solves this one, even if your if statement example wasnt acceptable and it looks fine to me
    – Ewan
    Commented Jan 21, 2019 at 23:06
  • 5
    @Ewan No one uses the goto. When is the last time you looked at the Linux kernel source code?
    – John Douma
    Commented Jan 22, 2019 at 2:43
  • 6
    You use goto when the high level structure does not exist in your language. I sometimes wish there was a fallthru; keyword to get rid of that particular use of goto but oh well.
    – Joshua
    Commented Jan 22, 2019 at 4:12
  • 25
    In general terms, if you feel the need to use a goto in a high-level language like C#, then you've likely overlooked many other (and better) design and/or implementation alternatives. Highly discouraged.
    – code_dredd
    Commented Jan 22, 2019 at 7:44
  • 13
    Using "goto case" in C# is different from using a more general "goto", because it's how you accomplish explicit fallthrough.
    – Hammerite
    Commented Jan 22, 2019 at 10:49

12 Answers 12

179

I find the code hard to read with the goto statements. I would recommend structuring your enum differently. For example, if your enum was a bitfield where each bit represented one of the choices, it could look like this:

[Flags]
public enum ExampleEnum {
    One = 0b0001,
    Two = 0b0010,
    Three = 0b0100
};

The Flags attribute tells the compiler that you're setting up values which don't overlap. The code that calls this code could set the appropriate bit. You could then do something like this to make it clear what's happening:

if (myEnum.HasFlag(ExampleEnum.One))
{
    CallOne();
}
if (myEnum.HasFlag(ExampleEnum.Two))
{
    CallTwo();
}
if (myEnum.HasFlag(ExampleEnum.Three))
{
    CallThree();
}

This requires the code that sets up myEnum to set the bitfields properly and marked with the Flags attribute. But you can do that by changing the values of the enums in your example to:

[Flags]
public enum ExampleEnum {
    One = 0b0001,
    Two = 0b0010,
    Three = 0b0100,
    OneAndTwo = One | Two,
    OneAndThree = One | Three,
    TwoAndThree = Two | Three
};

When you write a number in the form 0bxxxx, you're specifying it in binary. So you can see that we set bit 1, 2, or 3 (well, technically 0, 1, or 2, but you get the idea). You can also name combinations by using a bitwise OR if the combinations might be frequently set together.

22
  • 5
    It appears so! But it must be C# 7.0 or later, I guess. Commented Jan 21, 2019 at 23:06
  • 32
    Anyway, one could also do it like this: public enum ExampleEnum { One = 1 << 0, Two = 1 << 1, Three = 1 << 2, OneAndTwo = One | Two, OneAndThree = One | Three, TwoAndThree = Two | Three };. No need to insist on C#7+. Commented Jan 21, 2019 at 23:44
  • 8
    You may use Enum.HasFlag
    – IMil
    Commented Jan 22, 2019 at 0:34
  • 13
    The [Flags] attribute doesn't signal anything to the compiler. This is why you still have to explicitly declare the enum values as powers of 2.
    – Joe Sewell
    Commented Jan 22, 2019 at 2:49
  • 19
    @UKMonkey I vehemently disagree. HasFlag is a descriptive and explicit term for the operation being performed and abstracts the implementation from the functionality. Using & because it is common across other languages makes no more sense than using a byte type instead of declaring an enum.
    – BJ Myers
    Commented Jan 22, 2019 at 18:28
137

IMO the root of the problem is that this piece of code shouldn't even exist.

You apparently have three independent conditions, and three independent actions to take if those conditions are true. So why is all that being funnelled into one piece of code that needs three Boolean flags to tell it what to do (whether or not you obfuscate them into an enum) and then does some combination of three independent things? The single responsibility principle seems to be having an off-day here.

Put the calls to the three functions where the belong (i.e. where you discover the need to do the actions) and consign the code in these examples to the recycle bin.

If there were ten flags and actions not three, would you extend this sort of code to handle 1024 different combinations? I hope not! If 1024 is too many, 8 is also too many, for the same reason.

9
  • 10
    As a point of fact, there are many well-designed APIs which have all kinds of flags, options, and assorted other parameters. Some may be passed on, some have direct effects, and yet others potentially be ignored, without breaking SRP. Anyway, the function could even be decoding some external input, who knows? Also, reductio ad absurdum often leads to absurd results, especially if the starting-point isn't even all that solid. Commented Jan 22, 2019 at 2:30
  • 9
    Upvoted this answer. If you just want to debate GOTO, that's a different question than the one you asked. Based on the evidence presented, you have three disjoint requirements, which should not be aggregated. From a SE standpoint, I submit that alephzero's is the correct answer.
    – user326452
    Commented Jan 22, 2019 at 5:04
  • 8
    @Deduplicator One look at this mishmash of some but not all combinations of 1,2&3 shows that this is not a "well designed API".
    – user949300
    Commented Jan 22, 2019 at 5:39
  • 4
    So much this. The other answers don't really improve on what's in the question. This code needs a rewrite. There's nothing wrong with writing more functions.
    – user91988
    Commented Jan 22, 2019 at 22:39
  • 3
    I love this answer, especially "If 1024 is too many, 8 is also too many". But it is essentially "use polymorphism", isn't it? And my (weaker) answer is getting a lot of crap for saying that. Can I hire your P.R. person? :-)
    – user949300
    Commented Jan 23, 2019 at 1:08
29

Never use gotos is one of the "lies to children" concepts of computer science. It's the right advice 99% of the time, and the times it isn't are so rare and specialized that it's far better for everyone if it's just explained to new coders as "don't use them".

So when should they be used? There are a few scenarios, but the basic one you seem to be hitting is: when you're coding a state machine. If there's no better more organized and structured expression of your algorithm than a state machine, then its natural expression in code involves unstructured branches, and there isn't a lot that can be done about that which doesn't arguably make the structure of the state machine itself more obscure, rather than less.

Compiler writers know this, which is why the source code for most compilers that implement LALR parsers* contain gotos. However, very few people will ever actually code their own lexical analyzers and parsers.

* - IIRC, it's possible to implement LALL grammars entirely recursive-descent without resorting to jump tables or other unstructured control statements, so if you're truly anti-goto, this is one way out.


Now the next question is, "Is this example one of those cases?"

What I'm seeing looking it over is that you have three different possible next states depending on the processing of the current state. Since one of them ("default") is just a line of code, technically you could get rid of it by tacking that line of code on the end of the states it applies to. That would get you down to 2 possible next states.

One of the remaining ones ("Three") is only branched to from one place that I can see. So you could get rid of it the same way. You'd end up with code that looks like this:

switch (exampleValue) {
    case OneAndTwo: i += 3 break;
    case OneAndThree: i += 4 break;
    case Two: i += 2 break;
    case TwoAndThree: i += 5 break;
    case Three: i += 3 break;
    default: i++ break;
}

However, again this was a toy example you provided. In cases where "default" has a non-trivial amount of code in it, "three" is transitioned to from multiple states, or (most importantly) further maintainance is likely to add or complicate the states, you'd honestly be better off using gotos (and perhaps even getting rid of the enum-case structure that hides the state-machine nature of things, unless there's some good reason it needs to stay).

17
  • 2
    @PerpetualJ - Well, I'm certainly glad you found something cleaner. It might still be interesting, if that really is your situation, to try it out with the gotos (no case or enum. just labeled blocks and gotos) and see how they compare in readability. That being said, the "goto" option doesn't just have to be more readable, but enough more readable to stave off arguments and ham-handed "fix" attempts from other developers, so it seems likely you found the best option.
    – T.E.D.
    Commented Jan 22, 2019 at 16:16
  • 4
    I disbelieve that you'd be "better off using gotos" if "further maintenance is likely to add or complicate the states". Are you really claiming that spaghetti code is easier to maintain than structured code? This goes against ~40 years of practice.
    – user949300
    Commented Jan 22, 2019 at 20:10
  • 5
    @user949300 - Not practice against building state machines, it doesn't. You can simply read my previous comment on this answer for a real-world example. If you try to use C case fall-throughs, which impose an artificial structure (their linear order) on a state machine algorithm that has no such restriction inherently, you'll find yourself with geometrically increasing complexity every time you have to rearrange all your orderings to accommodate a new state. If you just code states and transitions, like the algorithm calls for, that doesn't happen. New states are trivial to add.
    – T.E.D.
    Commented Jan 22, 2019 at 20:54
  • 8
    @user949300 - Again, "~40 years of practice" building compilers shows that gotos are actually the best way for parts of that problem domain, which is why if you look at compiler source code, you'll almost always find gotos.
    – T.E.D.
    Commented Jan 22, 2019 at 21:02
  • 4
    @user949300 Spaghetti code doesn't just inherently appear when using specific functions or conventions. It can appear in any language, function, or convention at any time. The cause is using said language, function, or convention in an application that it wasn't intended for and making it bend over backwards to pull it off. Always use the right tool for the job, and yes, sometimes that means using goto.
    – Abion47
    Commented Jan 23, 2019 at 0:53
26

The best answer is use polymorphism.

Another answer, which, IMO, makes the if stuff clearer and arguably shorter:

if (One || OneAndTwo || OneAndThree)
  CallOne();
if (Two || OneAndTwo || TwoAndThree)
  CallTwo();
if (Three || OneAndThree || TwoAndThree)
  CallThree();

goto is probably my 58th choice here...

7
  • 5
    +1 for suggesting polymorphism, though ideally that might simplify the client code down to just "Call();", using tell don't ask -- to push the decision logic away from the consuming client and into the class mechanism and hierarchy.
    – Erik Eidt
    Commented Jan 21, 2019 at 23:58
  • 12
    Proclaiming anything the best sight unseen is certainly very brave. But without additional info, I suggest a bit of scepticism and keeping an open mind. Perhaps it suffers from combinatorial explosion? Commented Jan 22, 2019 at 0:15
  • Wow, I think this is the first time I've ever been downvoted for suggesting polymorphism. :-)
    – user949300
    Commented Jan 22, 2019 at 0:36
  • 25
    Some people, when faced with a problem, say "I'll just use polymorphism". Now they have two problems, and polymorphism. Commented Jan 22, 2019 at 11:44
  • 8
    I actually did my Master's thesis on using runtime polymorphism to get rid of the gotos in compilers' state machines. This is quite doable, but I've since found in practice its not worth the effort in most cases. It requires a ton of OO-setup code, all of which of course can end up with bugs (and which this answer omits).
    – T.E.D.
    Commented Jan 22, 2019 at 16:01
10

Why not this:

public enum ExampleEnum {
    One = 0, // Why not?
    OneAndTwo,
    OneAndThree,
    Two,
    TwoAndThree,
    Three
}
int[] COUNTS = { 1, 3, 4, 2, 5, 3 }; // Whatever

int ComputeExampleValue(int i, ExampleEnum exampleValue) {
    return i + COUNTS[(int)exampleValue];
}

OK, I agree, this is hackish (I'm not a C# developer btw, so excuse me for the code), but from the point of view of efficiency this is must? Using enums as array index is valid C#.

4
  • 3
    Yes. Another example of replacing code with data (which is usually desirable, for speed, structure and maintainability). Commented Jan 22, 2019 at 14:07
  • 2
    That's an excellent idea for the most recent edition of the question, no doubt. And it can be used to go from the first enum (a dense range of values) to the flags of more or less independent actions which have to be done in general. Commented Jan 22, 2019 at 14:46
  • I do not have access to a C# compiler, but maybe the code can be made safer using this kind of code: int[ExempleEnum.length] COUNTS = { 1, 3, 4, 2, 5, 3 }; ? Commented Jan 28, 2019 at 13:50
  • Now I add Four as the second enum value…
    – gnasher729
    Commented Jul 29, 2021 at 9:26
7

If you can't or don't want to use flags, use a tail recursive function. In 64bit release mode, the compiler will produce code which is very similar to your goto statement. You just don't have to deal with it.

int ComputeExampleValue(int i, ExampleEnum exampleValue) {
    switch (exampleValue) {
        case One: return i + 1;
        case OneAndTwo: return ComputeExampleValue(i + 2, ExampleEnum.One);
        case OneAndThree: return ComputeExampleValue(i + 3, ExampleEnum.One);
        case Two: return i + 2;
        case TwoAndThree: return ComputeExampleValue(i + 2, ExampleEnum.Three);
        case Three: return i + 3;
   }
}
1
  • That’s a unique solution! +1 I don’t think I need to do it this way, but great for future readers!
    – Taco
    Commented Jan 22, 2019 at 13:12
2

The accepted solution is fine and is a concrete solution to your problem. However, I would like to posit an alternative, more abstract solution.

In my experience, the use of enums to define the flow of logic is a code smell as it is often a sign of poor class design.

I ran into a real world example of this happening in code that I worked on last year. The original developer had created a single class which did both import and export logic, and switched between the two based on an enum. Now the code was similar and had some duplicate code, but it was different enough that doing so made the code significantly more difficult to read and virtually impossible to test. I ended up refactoring that into two separate classes, which simplified both and actually let me spot and eliminate a number of unreported bugs.

Once again, I must state that using enums to control the flow of logic is often a design problem. In the general case, Enums should be used mostly to provide type-safe, consumer-friendly values where the possible values are clearly defined. They're better used as a property (for example, as a column ID in a table) than as a logic control mechanism.

Let's consider the problem presented in the question. I don't really know the context here, or what this enum represents. Is it drawing cards? Drawing pictures? Drawing blood? Is order important? I also do not know how important performance is. If performance or memory is critical, then this solution is probably not going to be the one you want.

In any case, let's consider the enum:

// All possible combinations of One - Eight.
public enum ExampleEnum {
    One,
    Two,
    TwoOne,
    Three,
    ThreeOne,
    ThreeTwo,
    ThreeOneTwo
}

What we have here are a number of different enum values which are representing different business concepts.

What we could use instead are abstractions to simplify things.

Let us consider the following interface:

public interface IExample
{
  void Draw();
}

We can then implement this as an abstract class:

public abstract class ExampleClassBase : IExample
{
  public abstract void Draw();
  // other common functionality defined here
}

We can have a concrete class to represent Drawing One, two and three (which for the sake of argument have different logic). These could potentially use the base class defined above, but I'm assuming that the DrawOne concept is different from the concept represented by the enum:

public class DrawOne
{
  public void Draw()
  {
    // Drawing logic here
  }
}

public class DrawTwo
{
  public void Draw()
  {
    // Drawing two logic here
  }
}

public class DrawThree
{
  public void Draw()
  {
    // Drawing three logic here
  }
}

And now we have three separate classes which may be composed to provide the logic for the other classes.

public class One : ExampleClassBase
{
  private DrawOne drawOne;

  public One(DrawOne drawOne)
  {
    this.drawOne = drawOne;
  }

  public void Draw()
  {
    this.drawOne.Draw();
  }
}

public class TwoOne : ExampleClassBase
{
  private DrawOne drawOne;
  private DrawTwo drawTwo;

  public One(DrawOne drawOne, DrawTwo drawTwo)
  {
    this.drawOne = drawOne;
    this.drawTwo = drawTwo;
  }

  public void Draw()
  {
    this.drawOne.Draw();
    this.drawTwo.Draw();
  }
}

// the other six classes here

This approach is far more verbose. But it does have advantages.

Consider the following class, which contains a bug:

public class ThreeTwoOne : ExampleClassBase
{
  private DrawOne drawOne;
  private DrawTwo drawTwo;
  private DrawThree drawThree;

  public One(DrawOne drawOne, DrawTwo drawTwo, DrawThree drawThree)
  {
    this.drawOne = drawOne;
    this.drawTwo = drawTwo;
    this.drawThree = drawThree;
  }

  public void Draw()
  {
    this.drawOne.Draw();
    this.drawTwo.Draw();
  }
}

How much simpler is it to spot the missing drawThree.Draw() call? And if order is important, the order of the draw calls is also very easy to see and follow.

Disadvantages to this approach:

  • Every one of the eight options presented requires a separate class
  • This will use more memory
  • This will make your code superficially larger
  • Sometimes this approach is not possible, though some variation on it might be

Advantages to this approach:

  • Each of these classes is completely testable; because
  • The cyclomatic complexity of the draw methods is low (and in theory I could mock the DrawOne, DrawTwo or DrawThree classes if necessary)
  • The draw methods are understandable - a developer does not have to tie their brain in knots working out what the method does
  • Bugs are easy to spot and difficult to write
  • Classes compose into more high level classes, meaning that defining a ThreeThreeThree class is easy to do

Consider this (or a similar) approach whenever you feel the need to have any complex logic control code written into case statements. Future you will be happy you did.

3
  • This is a very well written answer and I agree entirely with the approach. In my particular case something this verbose would be overkill to the infinite degree considering the trivial code behind each. To put it in perspective, imagine the code is simply performing a Console.WriteLine(n). That is practically the equivalent of what the code I was working with was doing. In 90% of all other cases, your solution is definitely the best answer when following OOP.
    – Taco
    Commented Feb 18, 2019 at 13:31
  • Yeah fair point, this approach is not useful in every situation. I wanted to put it here because it's common for developers to get fixated on one particular approach to solving a problem and sometimes it pays to step back and look for alternatives.
    – Stephen
    Commented Feb 18, 2019 at 23:14
  • I completely agree with that, it’s actually the reason I ended up here because I was trying to solve a scalability issue with something another developer wrote out of habit.
    – Taco
    Commented Feb 18, 2019 at 23:24
0

If you are intent on using a switch here your code will actually be faster if you handle each case separately

switch(exampleValue)
{
    case One:
        i++;
        break;
    case Two:
        i += 2;
        break;
    case OneAndTwo:
    case Three:
        i+=3;
        break;
    case OneAndThree:
        i+=4;
        break;
    case TwoAndThree:
        i+=5;
        break;
}

only a single arithmetic operation is performed in each case

also as others have stated if you are considering using gotos you should probably rethink your algorithm (though I will concede that C#s lack of case fall though could be a reason to use a goto). See Edgar Dijkstra's famous paper "Go To Statement Considered Harmful"

3
  • 3
    I would say this is a great idea for someone faced with a simpler matter so thank you for posting it. In my case it isn’t so simple.
    – Taco
    Commented Jan 22, 2019 at 12:50
  • Also, we don’t exactly have the problems today that Edgar had at the time of writing his paper; most people nowadays don’t even have a valid reason to hate the goto aside from it makes code hard to read. Well, in all honesty, if it’s abused then yes, otherwise it’s trapped to the containing method so you can’t really make spaghetti code. Why spend effort to work around a feature of a language? I would say that goto is archaic and that you should be thinking of other solutions in 99% of use cases; but if you need it, that’s what it’s there for.
    – Taco
    Commented Jan 22, 2019 at 12:55
  • This won't scale well when there are many booleans.
    – user949300
    Commented Jan 23, 2019 at 1:05
0

For your particular example, since all you really wanted from the enumeration was a do/do-not indicator for each of the steps, the solution that rewrites your three if statements is preferable to a switch, and it is good that you made it the accepted answer.

But if you had some more complex logic that did not work out so cleanly, then I still find the gotos in the switch statement confusing. I would rather see something like this:

switch (enumVal) {
    case ThreeOneTwo: DrawThree; DrawTwo; DrawOne; break;
    case ThreeTwo:    DrawThree; DrawTwo; break;
    case ThreeOne:    DrawThree; DrawOne; break;
    case TwoOne:      DrawTwo; DrawOne; break;
    case Two:         DrawTwo; break;
    default:          DrawOne; break;
}

This is not perfect but I think it's better this way than with the gotos. If the sequences of events are so long and duplicate each other so much that it really doesn't make sense to spell out the complete sequence for each case, I'd prefer a subroutine rather than goto in order to reduce duplication of code.

0

I’m not sure that anyone really has a reason to hate the goto keyword these days. It’s definitely archaic and not needed in 99% of use cases, but it is a feature of the language for a reason.

The reason to hate the goto keyword is code like

if (someCondition) {
    goto label;
}

string message = "Hello World!";

label:
Console.WriteLine(message);

Oops! That's clearly not going to work. The message variable is not defined in that code path. So C# won't pass that. But it might be implicit. Consider

object.setMessage("Hello World!");

label:
object.display();

And assume that display then has the WriteLine statement in it.

This kind of bug can be hard to find because goto obscures the code path.

This is a simplified example. Assume that a real example would not be nearly as obvious. There might be fifty lines of code between label: and the use of message.

The language can help fix this by limiting how goto can be used, only descending out of blocks. But the C# goto is not limited like that. It can jump over code. Further, if you are going to limit goto, it's just as well to change the name. Other languages use break for descending out of blocks, either with a number (of blocks to exit) or a label (on one of the blocks).

The concept of goto is a low level machine language instruction. But the whole reason why we have higher level languages is so that we are limited to the higher level abstractions, e.g. variable scope.

All that said, if you use the C# goto inside a switch statement to jump from case to case, it's reasonably harmless. Each case is already an entry point. I still think that calling it goto is silly in that situation, as it conflates this harmless usage of goto with more dangerous forms. I would much rather that they use something like continue for that. But for some reason, they forgot to ask me before they wrote the language.

1
  • 2
    In the C# language you cannot jump over variable declarations. For proof, launch a console application and enter the code you provided in your post. You'll get a compiler error in your label stating that the error is use of unassigned local variable 'message'. In languages where this is allowed it is a valid concern, but not in the C# language.
    – Taco
    Commented Jan 23, 2019 at 17:43
0

When you have this many choices (and even more, as you say), then maybe it's not code but data.

Create a dictionary mapping enum values to actions, expressed either as functions or as a simpler enum type representing the actions. Then your code can be boiled down to a simple dictionary lookup, followed by either calling the function value or a switch on the simplified choices.

-7

Use a loop and the difference between break and continue.

do {
  switch (exampleValue) {
    case OneAndTwo: i += 2; break;
    case OneAndThree: i += 3; break;
    case Two: i += 2; continue;
    case TwoAndThree: i += 2;
    // dropthrough
    case Three: i += 3; continue;
    default: break;
  }
  i++;
} while(0);

Not the answer you're looking for? Browse other questions tagged or ask your own question.