Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

What is so bad about [Flags] enum? It does not support IEnumerable<T> anyhow, so to get one we need to use syntax like (see):

new HashSet<WhatToInclude>(new []{ 
    WhatToInclude.Cleaners, 
    WhatToInclude.Managers}

Here is the helper class Flag<T> to discuss. We can use it this way:

class Color : Flag<Color>
{
    public static readonly Color Red = new Color();
    public static readonly Color Green = new Color();
    public static readonly Color Yellow = new Color();
}

Manipulation and testing:

Color c = Color.None;
c += Color.Yellow + Color.Red;
c -= Color.Yellow + Color.Green;

// it implements IEnumerable<T>
bool t1 = c.Contains(Color.Red) // true
bool t2 = c.Contains(Color.None) // always false

Refactoring of enumeration to classes is half way done here already :)

Library code:

abstract class Flag<T> : IEnumerable<T> 
    where T : Flag<T>, new()
{
    public static T None = new T() { Flags = new T[0] };

    protected Flag()
    {
        Flags = new[] { (T)this };
    }

    IEnumerable<T> Flags { get; set; }
    public IEnumerator<T> GetEnumerator() => Flags.GetEnumerator();
    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();

    public static T operator+(Flag<T> left, Flag<T> right)
    {
        return new T()
        {
            Flags = left.Concat(right).Distinct().ToArray()
        };
    }

    public static T operator-(Flag<T> left, Flag<T> right)
    {
        return new T()
        {
            Flags = left.Except(right).ToArray()
        };
    }
}

Extra example

class FamilyMember : Flag<FamilyMember>
{
    public static readonly FamilyMember Me = new FamilyMember() { Name = "Dmitry" };
    public static readonly FamilyMember Wife = new FamilyMember() { Name = "Julia" };
    public static readonly FamilyMember Cat = new FamilyMember() { Name = "Willy" };
    public static readonly FamilyMember All = Me + Wife + Cat;

    public string Name { get; private set; }
}

We can use it in a very simple way:

foreach (var fm in FamilyMember.All - FamilyMember.Cat)
            Console.WriteLine($"Hello {fm.Name}!");

It is much more than just an Enum.

share|improve this question
2  
Can you give some context as to why you think this is going to be useful? I'm struggling to see what the point of it is – RobH Jan 25 at 14:14
    
Why would you want an enum to support IEnumerable<T>? – David Arno Jan 25 at 14:41
    
Please see an example above. – Dmitry Nogin Jan 25 at 14:57
3  
Enums aren't "things" like objects. What you're asking is similar to asking whether or not you could add LINQ to floats or decimals. Yes, you could do it, but it doesn't make sense; there's no reason you couldn't create a static class with the utilities you need that would have the pleasant effect of not blowing up type safety – Dan Pantry Jan 25 at 16:18
    
@DanPantry - Enums are countable, floats are not. Enum are refactorable to classes which are definitely things. Sorry, I can not agree. – Dmitry Nogin Jan 25 at 16:33

I'm not going to say I'd use this, nor that I wouldn't.

I think it's a clever1 use of generics, and turns C# enums into something else that could possibly be called JavaIshEnum; being an actual class, you significantly blur the line between classes and enums, but you also effectively work around the annoying absence of a generic type constraint in the C# compiler, that can constraint enum types.

Or do you?

public class Kaboom : Flag<SomeClass>
{
    ...
}

This derived type can be legally constructed... and shouldn't be allowed to. Of course, compile-time check is going to be hard (impossible?) - but your code is missing a run-time check.

I'd make the base constructor throw and forbid the creation of a non-enum T:

protected Flag()
{
    if (!typeof(T).IsEnum)
    {
        throw new ArgumentException("Non-enum type is not a valid type argument.");
    }
    Flags = new[] { (T)this };
}

I don't like this:

IEnumerable<T> Flags { get; set; }
public IEnumerator<T> GetEnumerator() => Flags.GetEnumerator();
IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();

You're only specifying access modifiers when you're overriding the default. I would have liked to see this:

private IEnumerable<T> Flags { get; set; }
public IEnumerator<T> GetEnumerator() => Flags.GetEnumerator();
private IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();

And then raise the question, why does Flags need to be an auto-property? If it's private, wouldn't just the backing field be enough? I don't get the point of private properties.. I just don't.

I also don't like that you have private and public members intertwined. Regroup them.


1 is that a good thing?

share|improve this answer
    
@mats-mug - Actually, T is not intended to be enum, and can not be (see usage example above). The class is used like a enum syntactically with extra features but is not a enum or compatible to it. typeof(T).IsEnum == false. – Dmitry Nogin Jan 25 at 17:27
1  
Well, there is a class constraint. Have a look at the code. Flag<T> is a class and T is a Flag<T> :) – Dmitry Nogin Jan 25 at 17:30
1  
Wow your code is even more confusing at second read than at first glance. – Mat's Mug Jan 25 at 17:31
2  
Let's agree to disagree then. A mutable IEnumerable<T> doesn't look like something I'd cheerfully expose. If the accessibility needs to change, there's a design flaw IMO. But don't mind me, I'm just a mug. – Mat's Mug Jan 25 at 17:41
1  
@mats-mug Yep, I would also fight for keeping this one private for sure. It is just a reasonable default approach to define private properties, not fields, if we can choose I think. Framework Design Guidelines says that .NET authors consider field/property distinction as a serious design flaw now, they would prefer to just have properties. – Dmitry Nogin Jan 25 at 17:47

I'm going to take your example to a silly extreme to illustrate a point:

foreach (var f in FamilyMember.All - FamilyMember.Cat)
{
    foreach (var f1 in f)
    {
        foreach (var f2 in f1)
        {
            foreach (var f3 in f2)
            {
                foreach (var f4 in f3)
                {
                    foreach (var f5 in f4)
                    {
                        foreach (var f6 in f5)
                        {
                            Console.WriteLine($"Hello {f6.Name}!");
                        }
                    }
                }
            }
        }
    }
}

If all of the Flag<T> implement IEnumerable<T> how do I know when to stop enumerating? I think it's a potentially confusing API design.

There's also no way to go back to the property name as far as I can tell - in your colour example how do I know what colours are in it?

I don't see what all of this buys you to be completely honest.

share|improve this answer
    
It is always flat (1 level collection), so it is not so bad. You can check varable c of type Color this way: "c == Color.Red" or "c.Contains(Color.Red)" or "c.Any(cc => cc == Color.Red)", etc. – Dmitry Nogin Jan 25 at 15:15
1  
@DmitryNogin - you know that but your code doesn't communicate it. What about getting the label? E.g. in your first test, how do I know which colours are in the final thing? – RobH Jan 25 at 15:21
3  
@DmitryNogin - so all of this extra power results in nearly the exact same code I would have typed anyway? Why is x.Contains() better that Enum.HasFlag()? – RobH Jan 25 at 15:30
1  
@DmitryNogin - Completely agree - I have very few enums in any of my codebases. I have just never needed to rengineer Flags. I don't see how what you have is anything other than a collection. – RobH Jan 25 at 15:43
2  
@DmitryNogin Statefulness doesn't belong in enums. Enums are - literally - enumerations of values. Those enumerations are 1) fixed 2) known at compile time and 3) just values with names associated with them. It sounds like what you want is a Java enum in C# - that's not going to do. The two concepts simply are not comparable. – Dan Pantry Jan 25 at 16:47

Personally, I would not want to use any "magic enum" class, simply because in the end of the day it is not enum. And therefore any code, which takes Enum as an argument, fetches an attribute or calls GetType().IsEnum won't work for this class. In rare cases where HasFlag method is not enough, and you actually need to enumerate flags, some static/extension method (say, ToEnumerable) should do a better job.

share|improve this answer

What is so bad about [Flags] enum? It does not support IEnumerable ...

You can iterate flagged enums with the following extension method:

public static class EnumExtensions
{
    public static IEnumerable<TEnum> AsEnumerable<TEnum>(this TEnum flaggedEnum)
        where TEnum : struct, IConvertible
    {
        if (!typeof(TEnum).IsEnum)
            throw new ArgumentException("TEnum must be an enumerated type");

        Enum flaggs = flaggedEnum as Enum;
        foreach (TEnum val in Enum.GetValues(typeof(TEnum)))
        {
            if (flaggs.HasFlag(val as Enum))
                yield return val;
        }
    }
}

Using that extension, the flagged enum

[Flags]
public enum FamilyMember
{
    Me = 1,
    Wife  = 2,
    Cat = 4,
    All = 7
}

can be iterated via:

// foreach (var value in (FamilyMember.Me | FamilyMember.Wife).AsEnumerable()) or
foreach (var value in (FamilyMember.All ^ FamilyMember.Cat).AsEnumerable())
    Console.WriteLine($"Hello {value}!");
// Output:
// Hello Me!
// Hello Wife!
share|improve this answer
    
Thanks. I would call this method AsEnumerable. – Dmitry Nogin May 25 at 17:32
    
Yes, AsEnumerable sounds better better. Thanks. – JanDotNet May 25 at 18:12

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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