10

Look at the code snippet:

This is what I normally do when coding against an enum. I have a default escape with an InvalidOperationException (I do not use ArgumentException or one of its derivals because the coding is against a private instance field an not an incoming parameter).

I was wondering if you fellow developers are coding also with this escape in mind....

public enum DrivingState {Neutral, Drive, Parking, Reverse};

public class MyHelper
{
    private DrivingState drivingState = DrivingState.Neutral;

    public void Run()
    {
        switch (this.drivingState)
        {
            case DrivingState.Neutral:
                DoNeutral();
                break;
            case DrivingState.Drive:
                DoDrive();
                break;
            case DrivingState.Parking:
                DoPark();
                break;
            case DrivingState.Reverse:
                DoReverse();
                break;
            default:
                throw new InvalidOperationException(
                    string.Format(CultureInfo.CurrentCulture, 
                    "Drivestate {0} is an unknown state", this.drivingState));
        }
    }
}

In code reviews I encounter many implementations with only a break statement in the default escape. It could be an issue over time....

12 Answers 12

15

Your question was kinda vague, but as I understand it, you are asking us if your coding style is good. I usually judge coding style by how readable it is.

I read the code once and I understood it. So, in my humble opinion, your code is an example of good coding style.

10

There's an alternative to this, which is to use something similar to Java's enums. Private nested types allow for a "stricter" enum where the only "invalid" value available at compile-time is null. Here's an example:

using System;

public abstract class DrivingState
{
    public static readonly DrivingState Neutral = new NeutralState();
    public static readonly DrivingState Drive = new DriveState();
    public static readonly DrivingState Parking = new ParkingState();
    public static readonly DrivingState Reverse = new ReverseState();

    // Only nested classes can derive from this
    private DrivingState() {}

    public abstract void Go();

    private class NeutralState : DrivingState
    {
        public override void Go()
        {
            Console.WriteLine("Not going anywhere...");
        }
    }

    private class DriveState : DrivingState
    {
        public override void Go()
        {
            Console.WriteLine("Cruising...");
        }
    }

    private class ParkingState : DrivingState
    {
        public override void Go()
        {
            Console.WriteLine("Can't drive with the handbrake on...");
        }
    }

    private class ReverseState : DrivingState
    {
        public override void Go()
        {
            Console.WriteLine("Watch out behind me!");
        }
    }
}
10
  • This looks like a kind of over-engineering to me. Its more lines of code and adds more complexity. Can you explain the additional value ? Mar 11, 2009 at 7:59
  • If there was no invalid case with the regular enum... how is adding an invalid null case making it tighter?
    – womp
    Mar 11, 2009 at 8:05
  • Patrick: it makes the enum more object oriented - the behaviour is built into the type, rather than being in a helper class. Also, if I have a non-null reference of class DrivingState, I know it's a valid one. The disadvantage (without extra work) is the lack of switch support.
    – Jon Skeet
    Mar 11, 2009 at 8:09
  • This looks like overkill for the original example. But in practice you never get things as simple as the original example and this might be a good idea. I'd probably start with a case statement and refactor to this when I get more than one case statement on the same enum.
    – Mendelt
    Mar 11, 2009 at 8:10
  • In addition, the base type can have common functionality - e.g. the name of the state (passed by the subclasses in their constructors). Why should a fixed collection of values be represented by just numbers? :)
    – Jon Skeet
    Mar 11, 2009 at 8:10
7

I don't like this approach because the default case is untestable. This leads to reduced coverage in your unit tests, which while isn't necessarily the end of the world, annoys obsessive-compulsive me.

I would prefer to simply unit test each case and have an additional assertion that there are only four possible cases. If anyone ever added new enum values, a unit test would break.

Something like

[Test]
public void ShouldOnlyHaveFourStates()
{
    Assert.That(Enum.GetValues( typeof( DrivingState) ).Length == 4, "Update unit tests for your new DrivingState!!!");
}
4
  • 5
    Dude. Enums are just kinda typed integers. Just pass <code>(DrivingState)-1</code> to it for a test.
    – Patrick
    Mar 11, 2009 at 8:08
  • @Patrick - it wasn't an argument. But you could do nasty things like reflection to change the state (yeuch), or set the state via an internal method and [InternalsVisibleTo] - neither is attractive, though. Mar 11, 2009 at 8:12
  • 4
    The problem with that is that it doesn't help you fix the code. When you need to add a new state, you update the "Four States" test - but now you have a number of methods that silently do NOTHING. Throwing a WTF? exception at runtime is probably preferable. Mar 11, 2009 at 8:24
  • Yeah oke, the Do... methods are more illustrative. I have implementation method stub that throw NotImplementedExceptions though. I have omitted them for readability of the topic. Mar 11, 2009 at 8:43
4

That looks pretty reasonable to me. There are some other options, like a Dictionary<DrivingState, Action>, but what you have is simpler and should suffice for most simple cases. Always prefer simple and readable ;-p

4

This is probably going off topic, but maybe not. The reason the check has to be there is in case the design evolves and you have to add a new state to the enum.

So maybe you shouldn't be working this way in the first place. How about:

interface IDrivingState
{
    void Do();
}

Store the current state (an object that implements IDrivingState) in a variable, and then execute it like this:

drivingState.Do();

Presumably you'd have some way for a state to transition to another state - perhaps Do would return the new state.

Now you can extend the design without invalidating all your existing code quite so much.

Update in response to comment:

With the use of enum/switch, when you add a new enum value, you now need to find each place in your code where that enum value is not yet handled. The compiler doesn't know how to help with that. There is still a "contract" between various parts of the code, but it is implicit and impossible for the compiler to check.

The advantage of the polymorphic approach is that design changes will initially cause compiler errors. Compiler errors are good! The compiler effectively gives you a checklist of places in the code you need to modify to cope with the design change. By designing your code that way, you gain the assistence of a powerful "search engine" that is able to understand your code and help you evolve it by finding problems at compile-time, instead of leaving the problems until runtime.

3
  • Interfaces evolve very bad in time. Changing the interface is changing the contract and all implementers have to change also then. It has no additional value for my code snippet though. Mar 11, 2009 at 8:22
  • 2
    That's the whole point. The compiler forces you to address any problems caused by the design change. Contrast this with the enum/switch approach, which can only produce errors at runtime. Mar 11, 2009 at 10:06
  • @DanielEarwicker I wholeheartedly agree with your assertion about enum/switch statement. They are often the source of increased cyclomatic complexities and bugs. To help deal with that, I use a subclassable enum technique described here: tyreejackson.com/subclassable-enums Aug 3, 2015 at 0:52
3

I would use the NotSupportedException.

The NotImplementedException is for features not implemented, but the default case is implemented. You just chose not to support it. I would only recommend throwing the NotImplementedException during development for stub methods.

1
  • Well.. I think that's arbitrary, but NotSupportedException will go too. Both exceptions are system exceptions that will let the program die, so using NotSupported- or InvalidOperation exception will make no difference. Mar 11, 2009 at 11:27
1

I would suggest to use either NotImplementedException or better a custom DrivingStateNotImplementedException if you like to throw exceptions.

Me, I would use a default drivingstate for default (like neutral/stop) and log the missing driverstate (because it's you that missed the drivingstate, not the customer)

It's like a real car, cpu decides it misses to turn on the lights, what does it do, throw an exception and "break" all control, or falls back to a known state which is safe and gives a warning to the driver "oi, I don't have lights"

2
  • Duh... no way. Custom exceptions are not the right way to do. Let say I have 20 enums and 20 implementations against it. That would lead to 20 custom exceptions with no additional value. Mar 11, 2009 at 8:16
  • +1 for NotImplementedException, but you don't need a custom exception here.
    – Keith
    Mar 11, 2009 at 10:21
1

What you should do if you encounter an unhandled enum value of course depends on the situation. Sometimes it's perfectly legal to only handle some of the values.

If it's an error that you have an unhandles value you should definitely throw an exception just like you do in the example (or handle the error in some other way). One should never swallow an error condition without producing an indication that there is something wrong.

A default case with just a break doesn't smell very good. I would remove that to indicate the switch doesn't handle all values, and perhaps add a comment explaining why.

1

Clear, obvious and the right way to go. If DrivingState needs to change you may need to refactor.

The problem with all the complicated polymorphic horrors above is they force the encapsulation into a class or demand additional classes - it's fine when there's just a DrivingState.Drive() method but the whole thing breaks as soon as you have a DrivingState.Serialize() method that serializes to somewhere dependent on DrivingState, or any other real-world condition.

enums and switches are made for each other.

0
0

I'm a C programmer, not C#, but when I have something like this, I have my compiler set to warn me if not all enum cases are handled in the switch. After setting that (and setting warnings-as-errors), I don't bother with runtime checks for things that can be caught at compile time.

Can this be done in C#?

3
  • 1
    AFAIK, there is no check for enum coverage in C# Mar 11, 2009 at 7:49
  • you can determine switch coverage through unit testing tools example for Visual Studio is Pex, or you can have Visual Studio generate the switch statement for you (but then it can become out of date with changes to the enum). Mar 11, 2009 at 7:53
  • Ah, interesting. I think I'll stick with GCC, though :)
    – Mikeage
    Mar 11, 2009 at 10:54
0

I never use switch. The code similar to what you show was always a major pain point in most frameworks I used -- unextensible and fixed to a limited number of pre-defined cases.

This is a good example of what can be done with simple polymorphism in a nice, clean and extensible way. Just declare a base DrivingStrategy and inherit all version of driving logic from it. This is not over-engineering -- if you had two cases it would be, but four already show a need for that, especially if each version of Do... calls other methods. At least that's my personal experience.

I do not agree with Jon Skeet solution that freezes a number of states, unless that is really necessary.

0

I think that using enum types and therefore switch statements for implementing State (also State Design Pattern) is not a particularly good idea. IMHO it is error-prone. As the State machine being implemented becomes complex the code will be progressively less readable by your fellow programmers.

Presently it is quite clean, but without knowing the exact intent of this enum it is hard to tell how it will develop with time.

Also, I'd like to ask you here - how many operations are going to be applicable to DrivingState along with Run()? If several and if you're going to basically replicate this switch statement a number of times, it would scream of questionable design, to say the least.

9
  • Well.. the code around the switch statement is illustrative. Can you add some additional arguments why enums are not the right thing to do when implementing state? Mar 11, 2009 at 8:41
  • Patrick, consider that you have 10 different states and 20 events that each can (or cannot) be triggered in any of these states. Now, imagine the implementation of such a state machine in switch style. Do you think it is easy to understand, to debug, to modify? Mar 12, 2009 at 8:20
  • Tell me... what other implementation are you suggesting in the 10 state, 20 event scenario ? Mar 13, 2009 at 9:22
  • Patrick, my favorite (in C++) is new-in-place based solution. It goes like this: 1. You define a basic state class that is union of all the data and all the methods of all the states of your state machine. By default all methods produce some kind of "unimplemented" or "forbidden" message. Mar 16, 2009 at 9:11
  • 2. Then you subclass for each state of your state machine. In each class you only define ctor that does not have any parameters and override each relevant method. This way when you look at any state (which is a separate class) you immediately see what it allows to do and how. Mar 16, 2009 at 9:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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