Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Is an empty flag a bad habit?

I faced a very stupid bug a few days ago. It was caused by this enum I get from third-party library:

[Flags]
public enum MyStatus
{
    OKResponse = 0,
    ResponseTooBig = 1,
    ErrorMessage = 2,
    NoResponse = 4,
    ...
}

I am used to check flags this way:

if ((status & MyStatus.OKResponse) != 0) {...}

but it doesn't work for MyStatus.OKResponse, because it is zero. It is not a flag at all, it is an absence of all flags. Of course, when I found the bug, I realised OKResponse was the only non error status, so it really means "no errors, no flags". However, I really don't find it obvious.

Is it a bad habit defining 0 as one of values in flags enum? What is the recommended way? What is the best way to check flags, that would work with "no flags" flag, too?

like image 534
vojta Avatar asked Mar 14 '23 00:03

vojta


2 Answers

Is it a bad habit defining 0 as one of values in flags enum?

No, on the contrary, as the comments say, it is common to use 0 as a value for a given flag, and that is what an enum will default to if no different value is assigned to the first given value. As others said in the comments, it's also common to use Enum.None as the first value for an enum, which makes your intentions clearer to anyone else reading the code.

What is the recommended way?

There is no one way to do it, but I usually like to use the concise Enum.HasFlag method:

void Main()
{
    var status = MyStatus.ResponseTooBig | MyStatus.NoResponse;
    if (status.Equals(MyStatus.OKResponse))
        Console.WriteLine("Status is OKResponse");
    else 
        Console.WriteLine($"Has NoResponse?: {status.HasFlag(MyStatus.NoResponse)}");
}
like image 56
Yuval Itzchakov Avatar answered Mar 16 '23 00:03

Yuval Itzchakov


While it's a good idea to actually define a 0 value in your enum, it's a very bad idea for it to represent OK, because 0 is the default value and it is the only special integer that is implicitly convertible to an enum, so it can creep in easily:

MyStatus status = 0;

So the actual defining of a 0 value is not a problem, but it's a good idea to make it to represent a None for Flags enums or an Invalid value for normal enums.

Also, Flags enums should be used when the members can be combined, but from your example it seems they are mutually exclusive.

So I'd say the enum is badly designed, because as it is, any response has an OK flag and that's certainly a bug for a Flags enum, and if you can't change the definition, the only way would be to explicitly check if the status equals OKResponse:

if (status == MyStatus.OKResponse)
like image 35
Saeb Amini Avatar answered Mar 15 '23 23:03

Saeb Amini