0

I have the following enum:

[Flags]
public enum Status { Nominal, Modified, DirOneOnly, DirTwoOnly, DirOneNewest, DirTwoNewest }

I am attempting to see whether the Modified bit has been set to true and have tried the following methods:

if(_stateFlags.HasFlag(Status.Modified))
{
    //DoStuff
}   //Found out why this doesn't work after reading docs.

and

if((_stateFlags & Status.Modified) == Status.Modified)
{
    //DoStuff
}

The latter is the method that my further research led me to believe would work. However, when I do _stateFlags = Status.DirTwoOnly the above statement still seems to evaluate to true which is really puzzling me.

Am I doing something fundamentally wrong?

Fruchtzwerg
  • 10,999
  • 12
  • 40
  • 49
ScottishTapWater
  • 3,656
  • 4
  • 38
  • 81
  • 1
    You gave it the [Flags] attribute but the enums don't have unambiguous values. (int)Status = 3 could be DirTwoOnly or could be Nominal | Modified. You have to number them so they don't overlap bits. So 1, 2, 4, 8, 16, etc. – Hans Passant Feb 14 '17 at 14:22
  • Thanks @HansPassant I'd assumed they defaulted to powers of two. My bad. – ScottishTapWater Feb 14 '17 at 14:24
  • You might consider whether it is worthwhile to define your own struct that still just takes up 4 bytes but has custom methods on it like "IsModified", etc. Having to do bit arithmetic to a value to determine its meaning makes code that should read like the semantics -- "is the status modified?" -- read more like the mechanisms -- "is bit three of this bit array turned on?" – Eric Lippert Feb 14 '17 at 14:31
  • If you are going to stick with an enum, remember that flags enums should have a None equal to zero. – Eric Lippert Feb 14 '17 at 14:32
  • Thanks for that last point @Eric, although with the way this is used, there should be no possibility of a `None` and if this happens I'd want it to throw an error – ScottishTapWater Feb 14 '17 at 14:33
  • This is then another reason to avoid an enum. Zero is **always** a legal value for any enum; the language specification guarantees this fact. So your best bet then if you still want to use an enum is to have a zero value called `Illegal` or some such thing. That way you have a value you can test in your assertions; you want to be able to say `Debug.Assert(status != Status.Illegal);` – Eric Lippert Feb 14 '17 at 15:59

3 Answers3

7

You have several answers to your question explaining what is wrong. I would suggest taking a different approach entirely.

Flags enums have a very 1990s feel to them; if you think they look like they're there for COM interop, and if you think COM enums look like they are for compatibility with bit-twiddling code from the 1970s, you're right.

I would not express this logic as an enum in new code. I would take the time to write a custom struct that clearly expresses my actual semantics.

struct Status
{
  public static readonly Status None = default(Status);
  private Status(int bits) { this.bits = bits; }
  private int bits;
  private const int NominalBitMask  = 0b0001;
  private const int ModifiedBitMask = 0b0010;
  ... etc ...
  public bool IsNominal => (this.bits & NominalBitMask) != 0;
  public Status WithNominal(bool f) => 
    new Status(f ? (this.bits | NominalBitMask) : (this.bits & ~NominalBitMask));
  ... etc ...

And now you can use it like:

Status status = Status.None.WithNominal(true).WithModified(myFile.IsModified);
...
if (status.IsModified) ...

Is this more work up front? Sure, it's about twenty minutes more work up front. But you then never make any bit twiddling mistake ever again. You have a struct you can test independently of the logic that uses it. You have code that looks like what it means. You never have to worry about someone casting an integer to your enum type and having it full of nonsense. You can put custom logic in your type; suppose for example there are three-valued flags -- true, false or null, say -- those are hard to do in flags enums but you can add the logic easily in a custom type. And so on.

Eric Lippert
  • 647,829
  • 179
  • 1,238
  • 2,067
  • Would you recommend doing this for non-flag enums? Or would that be overkill? (assuming that there was no need to add any methods or properties to the type). – Newell Clark Feb 15 '17 at 05:03
6

You need to define the enum constants as powers of two.

[Flags]
public enum Status
{
    Nominal = 1,
    Modified = 2,
    DirOneOnly = 4,
    DirTwoOnly = 8,
    DirOneNewest = 16,
    DirTwoNewest = 32
}

class Program
{
    static void Main(string[] args)
    {
        Status s = new Status();
        s |= Status.Modified;
        if (s.HasFlag(Status.Modified))
        {
            Console.WriteLine("Modified!");
        }
    }
}
TVOHM
  • 2,740
  • 1
  • 19
  • 29
1

The code:

[Flags]
public enum Status { Nominal, Modified, DirOneOnly, DirTwoOnly, DirOneNewest, DirTwoNewest }

is equal to:

[Flags]
public enum Status
{ 
    Nominal      = 0,  // in bits: ... 0000 0000
    Modified     = 1,  //              0000 0001
    DirOneOnly   = 2,  //              0000 0010
    DirTwoOnly   = 3,  //              0000 0011 **common bit with Modified state **
    DirOneNewest = 4,
    DirTwoNewest = 5,

}

So as other people said, you need to use power of 2 for enum values.

apocalypse
  • 5,764
  • 9
  • 47
  • 95