Best practice for checking for an enum flag

Question

I noticed these two patterns for checking for an enum flag:

[Flags]
public enum PurchaseType
{
    None = 0,
    SalePrice = 2,
    RegularPrice = 4,
    Clearance = 8,
    CreditCard = 16
}

public void Test()
{
    PurchaseType type = PurchaseType.Clearance;
    type |= PurchaseType.CreditCard;

    // Practice 1
    if ((type & PurchaseType.Clearance) == PurchaseType.Clearance)
    {
        // Clearance item handling
    }

    // Practice 2
    if ((type & PurchaseType.CreditCard) != 0)
    {
        // Credit card item handling   
    }
}

Of the two ways of checking for an enum flag, which one is better w.r.t performance, readability, code health, and any other considerations I should make?

Thanks, Mohammed

Better for what? Readability? Performance? Phase of the moon? Something else? — Oded, May 13 '12 at 19:02
I vote for practice 1. How would you test for `PurchaseType.None` with practice 2? Edit: I guess you could do (type & PurchaseType.None) == 0, but then now your checks are not really consistent. — Tung, May 13 '12 at 19:02
@Tung Er, `None` is not a flag that can be tested for. `type & PurchaseType.None` is `0` for all values of `type`. You can never test for `None` so there's no point in worrying about how to do what cannot be done and is never done. — David Heffernan, May 13 '12 at 19:13
@Oded: The question is open-ended ("which one is better and why") so that other aspects to consider like "performance" and "phase of the moon" can be highlighted. :) — Mohammed Ali, May 13 '12 at 19:17
That makes it a _poor fit_ for Stack Overflow. Please read the [FAQ](http://stackoverflow.com/faq) — Oded, May 13 '12 at 19:19
@Oded - the trouble with people being against "unanswerable" "which one is better and why" questions, is that it is not always clear it is unanswerable when it is asked. The OP could have asked this question and had everyone agree "Practice n is better for the following reason, don't do practice m". This is actually a fair question. The fact the answer is actually "there is no compelling reason to chose between" them doesn't negate this question at all IMHO. In addition to this, it has given rise to an enlightening conversation around reflection in HasFlag - which is itself also v. useful. — Rob Levine, May 13 '12 at 19:31
@Oded: I am not sure if I agree that this is a "poor fit" for SO. I don't see how this comment in the FAQ "Chatty, open-ended questions diminish the usefulness of our site and push other questions off the front page." can be applied here. Are you referring to this statement? If so, do you really think that this comment also applies to my "open-ended" question? — Mohammed Ali, May 13 '12 at 19:31

score 17 · Accepted Answer · edited May 13 '12 at 19:09

17

.Net 4 introduces a HasFlag method that determines whether one or more bit fields are set in the current instance, this is by far the best practice:

type.HasFlag(PurchaseType.CreditCard);  // true

edited May 13 '12 at 19:09

Erik Philips

53,428
11
128
150

answered May 13 '12 at 19:04

Kamyar Nazeri

25,786
15
50
87

That looks easier to use. I saw this [connect issue](http://connect.microsoft.com/VisualStudio/feedback/details/554908/enum-hasflag-tremendously-bad-performance) regarding performance. I wonder if this was fixed in 4.0. – Mohammed Ali May 13 '12 at 19:10
3

A [blog entry](http://blogs.microsoft.co.il/blogs/bnaya/archive/2011/01/28/enum-hasflag-good-or-bad.aspx) regarding this and bad performance. It seems that this is not just some simple syntactic sugar. This method seems to employ reflection according to this post. – Mohammed Ali May 13 '12 at 19:13
I see many upvotes for this answer and now am curious. Let's say that there was a perf penalty for using this over others. Would readbability win here over performance? – Mohammed Ali May 13 '12 at 19:22
1

You want us to answer whether or not the performance penalty is significant for you?!! Only you can answer that. – David Heffernan May 13 '12 at 19:24
@MohammedAli - If your HasFlag is not performance critical I would be inclined to go with readability. However - I've got to be honest - until I saw the link to the blog article I didn't realise there was any potential issue. Now I look in reflector, I see it does indeed use reflection, to check that the two enum types match. – Rob Levine May 13 '12 at 19:26
1

If performance is an issue, an alternative to making your code unreadable is to consolidate it into a `HasFlag` method for each enum type you'd like to compare. If you use a different method name or signature, you can even make it an extension method, e.g. `public static bool HasFlagFast(this PurchaseType v, PurchaseType f) { return (v & f) == f; }` and called using `type.HasFlagFast(PurchaseType.CreditCard);` – Tim S. May 13 '12 at 21:02
Careful using this pattern with None==0, as `type.HasFlag(PurchaseType.None)` will always evaluate to true, when you probably mean `type == PurchaseType.None` – AaronLS Feb 19 '14 at 22:40

score 2 · Answer 2 · answered May 13 '12 at 19:03

2

I would choose the first one:

if ((type & PurchaseType.Clearance) == PurchaseType.Clearance)
{
    // Clearance item item handling
}

cause it clearly clams that you're checking for Clearance type presence.

answered May 13 '12 at 19:03

Tigran

61,654
8
86
123

3

Hmm, I don't like that much. I can see duplication. If you changed `Clearance` to one of the other flags but forgot to do it in both locations, then your code would compile, but it would not do what you want. `!=0` wins for me. – David Heffernan May 13 '12 at 19:12
@DavidHeffernan: agree (on dupplicate), but like the clear esplicit declaration. – Tigran May 13 '12 at 19:14
@David: I actually made this exact mistake when typing my question! – Mohammed Ali May 13 '12 at 19:23
@MohammedAli Sounds like you have your answer then. At least out of the two candidates in the question. – David Heffernan May 13 '12 at 19:25
@David: I don't see a way of accepting your comment as an answer :). – Mohammed Ali May 13 '12 at 19:36

score 1 · Answer 3 · answered May 13 '12 at 19:59

1

I personally would always prefer the clear readability of HasFlag.

However, out of the two options in the question I think !=0 is safer because it has no duplication. If you use your alternative then it's all too easy when maintenence coding to change one of of the flags and forget to change the other. And then you end up with this

if ((type & PurchaseType.Clearance) == PurchaseType.CreditCard)

answered May 13 '12 at 19:59

David Heffernan

601,492
42
1,072
1,490

in the context of performance. is there a cast involved in using the != 0 check? – Mohammed Ali May 13 '12 at 20:04
How could comparing against 0 have performance implications? – David Heffernan May 13 '12 at 20:18
Does the 0 value have to be casted up during the check? If so, is it any worse than the other option in my question? – Mohammed Ali May 13 '12 at 21:08
1

Why would a cast cost processor time? Casts between ints and enums are dealt with at compile time. – David Heffernan May 13 '12 at 21:12
I see so both options seem equally performant. – Mohammed Ali May 13 '12 at 23:00

score 1 · Answer 4 · answered May 13 '12 at 20:45

1

I would prefer (type & PurchaseType.CreditCard) != 0 because if you want to check for more than one bit then the right hand side becomes cumbersome. I trust in bit operations that the above will only be true if and only if the bit(s) is set.

answered May 13 '12 at 20:45

John Alexiou

28,472
11
77
133

Best practice for checking for an enum flag

4 Answers4