Vtgyjfy

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.

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).

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...

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#.

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;

   }

}

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.

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"

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.

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.

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.