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?

Hazel へいぜる
  • 1,165
  • 1
  • 8
  • 19
  • 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 Jan 21 '19 at 23:06
  • 1
    I personally have used goto in C# on many occasions. – whatsisname Jan 22 '19 at 01:29
  • 5
    @Ewan No one uses the goto. When is the last time you looked at the Linux kernel source code? – John Douma Jan 22 '19 at 02: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 Jan 22 '19 at 04:12
  • Possible duplicate of [Style for control flow with validation checks](https://softwareengineering.stackexchange.com/questions/148849/style-for-control-flow-with-validation-checks) – gnat Jan 22 '19 at 06:55
  • 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 Jan 22 '19 at 07:44
  • 13
    Using "goto case" in C# is different from using a more general "goto", because it's how you accomplish explicit fallthrough. – Hammerite Jan 22 '19 at 10:49
  • 1
    @Ewan your comment lacks a real argument. Peer pressure is not an argument. – mike3996 Jan 22 '19 at 12:01
  • 2
    "Pretty great and simple stuff right?" Uh, not really. It would be much simpler if you just had three boolean values and validated that all three are not set. Hardcoding combinations of boolean flags doesn't seem helpful here. – JimmyJames Jan 22 '19 at 15:16
  • 4
    I'm going to downvote not because you mention "goto", but because you keep changing the question, provide silly code, and are argumentative. IN the original question it appeared that order might matter, there was a function call not a trivial ++. And it's pretty clear that neither code is anywhere close to your "real code". – user949300 Jan 22 '19 at 15:42
  • 1
    @user949300 Good to know! I am unable to post my actual code for this case since it is not my own code; it is code that was written by a co-worker 15 years ago and I am refactoring the entire project per request. I apologize for changing the code up, but such is the process of ensuring everyone understands my concerns. I continuously forget that our community is very literal and as such even pseudo-code for examples is examined to the finest detail. – Hazel へいぜる Jan 22 '19 at 16:24
  • 1
    Thanks for the update. The Cytometric Complexity of your "goto" code is, er, immense, and it's very prone to bugs. IMO, your version 2 (`if`s) is far better, and your version 1 (separate `case`) is somewhat better. Both are much more clear and it's easier to debug and see correctness. – user949300 Jan 22 '19 at 16:41
  • 1
    How can you write such an `enum` without thinking "There must be an easier way!"? – D Drmmr Jan 22 '19 at 19:36
  • 2
    @DDrmmr I didn't write the original `enum`; I am refactoring code that is 15 years old. I saw it and instantly thought *there must be an easier way*. – Hazel へいぜる Jan 22 '19 at 19:45
  • 3
    @user949300 since we're not on Biology.SE, I assume you meant "cyclomatic" ;) – Quentin Jan 22 '19 at 21:43
  • @Quentin Oops! But, in my defense, I used to program Flow Cytometers, so guess my fingets just auto-spelled it that way. – user949300 Jan 23 '19 at 00:24
  • One particular real-world case where `goto` would be absolutely required is for the entry point of a fast base conversion routine. In C, libb64 does this by using a complicated switch statement with case labels nested inside sub-blocks and the gratuitous use of case fall through to achieve the same effect, but since C# doesn't support either of those things AFAIK, there's not really a better way of doing it in pure C# aside from goto. – AJMansfield Jan 23 '19 at 03:35
  • `I have to use a goto in that case since C# doesn't allow fall-through.` but C# does allow to fall through. – Tvde1 Jan 23 '19 at 10:49
  • I don't know the particular language, but can't you treat `enumValue` like a string? I mean `if (enumValue contains One) drawOne()` etc. should be enough. – Džuris Jan 23 '19 at 12:31
  • @AJMansfield I just did a quick check of this [libb64 implementation](https://github.com/transmission/libb64) and see no gotos. – user949300 Jan 23 '19 at 18:55
  • @user949300 If you re-read AJ's statement you'll see that they are stating that `libb64` does this without `goto` by using *a complicated switch statement with case labels nested inside sub-blocks and the gratuitous use of case fall through* to achieve the same effect. But that is `C` not `C#`. – Hazel へいぜる Jan 23 '19 at 19:00
  • @user949300 It uses a switch statement to jump to one of four points within the main while loop to start off decoding, and while C supports that particular construct, goto is required to implement the same type of control flow in C#. – AJMansfield Jan 23 '19 at 19:02
  • Using a lack of switch fall-through as a reason for a go-to doesn't really cut it--switch statements are borderline avoid-if-you-can as well. They happen to produce really tight code in C so there is value in that, but when you have other ways to write equivalent code, you should use them instead. – Bill K Jan 23 '19 at 19:06
  • 2
    I note that you say you have all eight combinations, but the enum only has seven members. You've forgotten "None". – Eric Lippert Jan 24 '19 at 18:17
  • What you have is essentially set of values where every possible subset is encoded as enum. This seems overly complicated way of doing something that could be achieved with much simpler use of bitmaps or plain old set data structures. – Roland Tepp Jan 24 '19 at 19:43
  • @EricLippert: Ah the joys of redacted code. It's entirely possible he can prove that "None" can't happen in the real code. Yet you do have a point. – Joshua Jan 24 '19 at 21:21
  • There is a lot going in with a `goto` besides jumping to a different line and resuming execution there. What are the rules regarding variable scope after a jump? If the program jumps out of a function what happens to the stack? There are cute cases where it acts like `break`, but in general it can have a lot of side effects. In C# it is relatively limited in support to safe functionality with defined behavior, but what about C? This is the C# specification: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/statements#the-goto-statement – le3th4x0rbot Jan 24 '19 at 21:51
  • "how dare you mention goto grr arggg" Citation needed. – Paul Draper Jan 26 '19 at 08:53
  • Why is there a paragraph devoted to saying why this question is not related to some other completely unrelated post? It is either a duplicate or is not a duplicate. Editing the post to say "it is not a duplicate because X" should NOT be considered evidence for not being a duplicate. If the post cannot be not a duplicate without such an edit, then it is a duplicate, period. The post should not be establishing some meta-argument on whether or not it should be closed for any reason. Every post will claim it should not be closed. Actually stating it is pointless. Remove that paragraph please. – user64742 Jan 26 '19 at 18:27

12 Answers12

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.

Andy
  • 2,003
  • 16
  • 22
user1118321
  • 4,969
  • 1
  • 17
  • 25
  • 1
    Does C# have binary constants? Usually I use hex for flags but this is a case where binary would be superior. – user949300 Jan 21 '19 at 23:04
  • 5
    It [appears so](https://stackoverflow.com/questions/594720/c-sharp-binary-literals)! But it must be C# 7.0 or later, I guess. – user1118321 Jan 21 '19 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+. – Deduplicator Jan 21 '19 at 23:44
  • 8
    You may use [Enum.HasFlag](https://docs.microsoft.com/en-us/dotnet/api/system.enum.hasflag?view=netframework-4.7.2) – IMil Jan 22 '19 at 00:34
  • 1
    @Deduplicator, since the exact bits don't matter, I'd suggest `One = 1 << 1, Two = 1 << 2, Three = 1 << 3` for a little more clarity. Though who knows, maybe using 1-based math will confuse the programmer? :-) – user949300 Jan 22 '19 at 00:35
  • Isn't Enum flags is exactly what you talking about in this answer? – Fabio Jan 22 '19 at 00:39
  • It would appear so! Thanks for all the help everyone! – user1118321 Jan 22 '19 at 02:15
  • 13
    [The `[Flags]` attribute doesn't signal anything to the compiler.](https://stackoverflow.com/a/8480/2137382) This is why you still have to explicitly declare the enum values as powers of 2. – Joe Sewell Jan 22 '19 at 02:49
  • 5
    @user Why would you suggest to use 2 for the value One? 1 shifted by 1 is 0b10 and 0b1. Deduplicators approach is how everybody has defined bit flags in C and co for decades and it's foolproof and simple. – Voo Jan 22 '19 at 14:23
  • 1
    This worked wonders to clean up the code! I no longer have incredibly long `if` conditions and was able to reduce to simply 8 flags (for the sake of keeping with the example `One - Eight`) and can combine them on assignment in any way I wish which allows for a highly fluid design! Thank you! – Hazel へいぜる Jan 22 '19 at 15:50
  • 3
    @IMil I think `&` is vastly more readable. It's common across languages; uses less characters and is perfectly clear if you know your bitwise operations (which everyone should) – UKMonkey Jan 22 '19 at 16:51
  • 1
    @Voo Because this is obviously not the real code. The constants are not literally named `One`, `Two`, etc. I'm guessing. – user1118321 Jan 22 '19 at 17:16
  • 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 Jan 22 '19 at 18:28
  • 4
    @user1118321 Well sure, but if I saw the code I'd be wondering why exactly you left out the first valid power of 2 and if there was a deeper reason for it. I mean we don't ignore the first value of arrays because some beginners might get confused by 0-indexing - why start here with it? – Voo Jan 22 '19 at 18:43
  • @BJMyers While I agree in terms of the descriptiveness of `HasFlag`, using an `&` does the exact same thing and is a common enough pattern that I wouldn't think less of anyone electing to use it instead. I could even see the argument that `&` is cleaner since `HasFlag` doesn't have a generic implementation and thus has to incorporate null and type checking, whereas there are very few simpler operations than a bitwise operation on two integral expressions. Personally, I've always felt the lack of generics on `HasValue` has made that method just feel clunky. – Abion47 Jan 23 '19 at 00:45
  • 1
    @Abion47: For a few releases now, the JIT knows `HasFlag` and emits optimal code without the cost of boxing. – Joey Jan 23 '19 at 07:13
  • Note that this is not the same as it will not bail after executing CallOne() – Emond Jan 23 '19 at 09:44
  • @BJMyers "abstracts the implementation from the functionality" apart from it really doesn't. It's a bit like saying that `if (x) { if (y) }` abstracts the implementation from `if (x || y)`. The latter being very much easier to read; and that is far more important than anything else. As for your enum and byte example; all the major languages support enum so it's a poor example at best. – UKMonkey Jan 23 '19 at 09:47
  • 1
    In any case, the general structure is still correct regardless of whether the actual test is `myEnum.HasFlag` or `&` or something else. The enum doesn't even necessarily need to be implemented as a bit mask, the important thing is that there's a non-verbose bit of code you can write (either a function call or otherwise) to tell you whether or not this enum value requires `CallOne` to be called. Then, the bit of code that implements "figure out from the enum value which functions to call" will look like that's what it's doing. – Steve Jessop Jan 23 '19 at 11:43
  • ... the remaining question is, "look like that to whom?". If you don't use `hasFlag` very often and are worried what corner cases it might introduce then of course `&` is clearer. If you don't use bitwise operations often then of course the method is clearer. So, decide what your readers are ignorant of, and then you can answer these pinhole readability questions. – Steve Jessop Jan 23 '19 at 11:48
  • @Joey I see that is the case for the JIT in the .NET Core runtime, but I can't find anything similar for .NET Framework. Also, the criteria for the optimization seems to be pretty vague. (Simple `myEnum.HasValue(MyEnum.SomeValue)` is obviously supported, but anything more complex may not be.) As such, I'd still advocate using `&` for the same reason I'd advocate using `===` over `==` in Javascript - you can either use the "user-friendly" way and assume it will work, or you can use the explicit way the language supports and most people will understand just fine while _knowing_ it will work. – Abion47 Jan 23 '19 at 15:53
  • 4
    Just a reminder: .NET guidelines strongly encourage flags enums to have a `None` element equal to zero. Remember, all `int` values can be stuffed into an enum, and `default(SomeEnum)` will give you the zero value, so it is helpful to have it named `None` for convenience. – Eric Lippert Jan 24 '19 at 18:20
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.

alephzero
  • 1,269
  • 1
  • 8
  • 8
  • So grateful for this answer! In my opinion the code is perfectly valid and genericized for this particular post. The question is in relation to the `goto` statement and not the concept of single responsibility. I have refactored the code to handle everything with four simple `if` statements. More to the point on your answer, there is nothing wrong with multiple if statements in the same method so long as they achieve a related goal to the method and simply alter the execution path. In my actual code each piece isn't actually a method call, but a single line execution. – Hazel へいぜる Jan 22 '19 at 02:02
  • 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. – Deduplicator Jan 22 '19 at 02: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. –  Jan 22 '19 at 05: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 Jan 22 '19 at 05:39
  • @user949300 The code doesn't have any meaningful names, so we cannot say why some combinations are special. There can be perfetly good reasons why some aren't (generally) used, or others get a more convenient label. We simply cannot say. – Deduplicator Jan 22 '19 at 09:45
  • @user949300 The 1,2, and 3 are placeholders here. They mean nothing other than to show that order of execution doesn’t matter. – Hazel へいぜる Jan 22 '19 at 12:48
  • 1
    As presented, it appears to be an interrupt handler, in which case the single responsibility is "interpret generic flag and dispatch to appropriate handler." Although certain branches having multiple statements does shed a little doubt on that interpretation, IMO... – Justin Time - Reinstate Monica Jan 22 '19 at 18:06
  • 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 Jan 22 '19 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 Jan 23 '19 at 01: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).

Peter Mortensen
  • 1,050
  • 2
  • 12
  • 14
T.E.D.
  • 1,069
  • 1
  • 9
  • 11
  • This is a very thorough answer! +1. I swapped to using the `Flags` option provided above because it is more fluid than the switch structure and turned out very clean. Yes, my code is more complex than the example given here, and your final paragraph applies completely. – Hazel へいぜる Jan 22 '19 at 15:43
  • 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. Jan 22 '19 at 16:16
  • You can "get your feet wet" with unstructured code without actually using goto, by letting some `case`s in a switch fall through instead of ending with `break;` You definitely should comment this, but the OP's example has multiple cases that could be arranged to fall through instead of doing a `goto`. It's readable by humans if two cases are actually related conceptually, and not just in a coincidence of implementation detail. – Peter Cordes Jan 22 '19 at 17:14
  • @PeterCordes - You probably missed this line in the question: "I have to use a goto in that case since C# doesn't allow fall-through. " – T.E.D. Jan 22 '19 at 18:37
  • Ah yes I did miss that. I wondered if maybe some languages would enforce structured programming and disallow case fallthrough. C and C++ let you, but modern compilers warn about it, and provide special syntax to indicate that you really did mean that to silence the warning. (Similar to a special way to write a `goto` to the next case.) [How to do an explicit fall-through in C](https://stackoverflow.com/q/44511436) shows an example of GNU C's `__attribute__ ((fallthrough));` on a case label. – Peter Cordes Jan 22 '19 at 18:42
  • @PeterCordes - C was (thankfully) pretty unique in that regard. However, it was also very popular, so a lot of languages that borrowed its syntax also borrowed that particular misfeature. I did once try creative (mis?) use of C switch fallthroughs to build a parser, but they turn out to be super-duper brittle. I'd extend the language to accept a new somewhat rare keyword, and the whole damn thing had to be redesigned. Discrete logical blocks of goto targets that can be maintained in isolation are just way better – T.E.D. Jan 22 '19 at 18:46
  • C began as a kind of portable assembly language, so it makes sense from that POV. But yeah as a high level language, it's kind of letting low-level details leak through. Except when you have multiple case labels on the same block of code, which is widely used and not considered evil. Syntax to allow multiple cases for one block but not fall-through past other statements would probably keep most modern C users happy these days, if it had existed from the start. But historically, well there's stuff like Duff's Device that was useful on machines with post-increment addressing... – Peter Cordes Jan 22 '19 at 18:55
  • 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 Jan 22 '19 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. Jan 22 '19 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. Jan 22 '19 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 Jan 23 '19 at 00:53
  • @T.E.D. I agree with you that case fallthroughs would rapidly become unmanageable for this case. But the bitset idea, explicit ifs, or, up to a limit, explicit case statements with no fall through, are all, IMO, better than a web of gotos. – user949300 Jan 23 '19 at 00:59
  • @user949300 You may want to consider a more specific term than "spaghetti code", it seems to have just devolved into ["code that's hard to maintain"](https://en.wikipedia.org/wiki/Spaghetti_code) over the years. I'd recommend describing the problem it causes instead; the only thing I get out of "spaghetti code" is a vote towards "a lot of people don't like this / find it hard to understand" which is weaker than ["here's a disaster I could have avoided"](https://gizmodo.com/the-typo-that-destroyed-a-nasa-rocket-1596004226) – jrh Jan 23 '19 at 14:24
  • 1
    "But Master, when will I know that I'm wise enough to use `goto` safely?" "When you no longer wish to use `goto`." – Shadur Jan 23 '19 at 22:20
  • This whole problem is created by not having Tail Call Elimination. If you are parsing data, the goto mess is very cleanly dealt with using tail calls. But without tail call elimination support, you can't write it. – Rob Jan 25 '19 at 06:39
  • I think everyone agrees that the greater evil is not "goto" but rather "backward goto". Forward goto can still be hard to read, but usually much less so. – Paul Draper Jan 26 '19 at 08:38
  • "Lies to children" yeah, I think that's the case, now that I'm reading the [Linux Kernel source](https://elixir.bootlin.com/linux/v4.13/source/net/ipv4/ip_output.c#L911) that has both forward and backward gotos quite commonly, even in avoidable cases, and even gotos jumping in and out of control blocks. It really doesn't bother me to be completely honest... I'd consider excessive state variables in a program to be a bigger problem. – jrh Feb 04 '19 at 15:13
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...

Tvde1
  • 302
  • 2
  • 11
user949300
  • 8,679
  • 2
  • 26
  • 35
  • 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 Jan 21 '19 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? – Deduplicator Jan 22 '19 at 00:15
  • Wow, I think this is the first time I've ever been downvoted for suggesting polymorphism. :-) – user949300 Jan 22 '19 at 00:36
  • 25
    Some people, when faced with a problem, say "I'll just use polymorphism". Now they have two problems, and polymorphism. – Lightness Races in Orbit Jan 22 '19 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. Jan 22 '19 at 16:01
  • @T.E.D. Agreed. In the revised question where you are simply incrementing an integer, OO might be overkill... :-) But in the general case something "OO like" works fine. In more dynamic languages like Python or JS there isn't even that much extra work to setup, though Java would be more work. – user949300 Jan 22 '19 at 16:35
  • Then case OneAndFour is added… – gnasher729 Jul 29 '21 at 09:24
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#.

  • 3
    Yes. Another example of replacing code with data (which is usually desirable, for speed, structure and maintainability). – Peter - Reinstate Monica Jan 22 '19 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. – Deduplicator Jan 22 '19 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 };` ? – Laurent Grégoire Jan 28 '19 at 13:50
  • Now I add Four as the second enum value… – gnasher729 Jul 29 '21 at 09: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;
   }
}
Philipp
  • 71
  • 2
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.

Stephen
  • 8,800
  • 3
  • 30
  • 43
  • 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. – Hazel へいぜる Feb 18 '19 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 Feb 18 '19 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. – Hazel へいぜる Feb 18 '19 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
    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. – Hazel へいぜる Jan 22 '19 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. – Hazel へいぜる Jan 22 '19 at 12:55
  • This won't scale well when there are many booleans. – user949300 Jan 23 '19 at 01: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.

David K
  • 475
  • 2
  • 8
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.

mdfst13
  • 320
  • 1
  • 6
  • 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. – Hazel へいぜる Jan 23 '19 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.

alexis
  • 667
  • 3
  • 7
-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);
BobDalgleish
  • 4,644
  • 5
  • 18
  • 23
QuentinUK
  • 97
  • 2