Viable Flags (Enum) ValueObject

Question

I was answering a question that led down a rabbit hole of creating an EnumValueObject with [Flags] capabilities.

I started with a simple EnumValuObject implementation:

public abstract class EnumValueObject {

    public abstract string Name { get; }

    public abstract int Value { get; }

    public override bool Equals(object obj) {
        var otherValue = obj as EnumValueObject;
        if (otherValue == null) {
            return false;
        }
        var typeMatches = GetType().Equals(obj.GetType());
        var valueMatches = Value.Equals(otherValue.Value);
        return typeMatches && valueMatches;
    }

    public override int GetHashCode() {
        return string.Join(",", Value, Name).GetHashCode();
    }

    public override string ToString() {
        return Name;
    }
}

Simple enough...

Then moved on to a generic FlagsValueObject to satisfy the desired functionality:

public abstract class FlagsValueObject<T> : EnumValueObject where T : FlagsValueObject<T> {
    protected IDictionary<int, string> Types = new Dictionary<int, string>();

    protected FlagsValueObject(int value, string name) {
        Types[value] = name;
    }

    protected FlagsValueObject() {

    }

    public override string Name {
        get {
            return string.Join(", ", Types.OrderBy(_ => _.Value).Select(_ => _.Value));
        }
    }

    public override int Value {
        get { return Types.Keys.Aggregate((a, b) => a + b); }
    }

    public abstract T And(T other);

    public virtual bool HasFlag(T flag) {
        var typeMatches = GetType().Equals(flag.GetType());
        return typeMatches && (Value & flag.Value) == flag.Value;
    }

    public virtual bool HasFlagValue(int value) {
        return (Value & value) == value;
    }

    public override int GetHashCode() {
        return Types.GetHashCode();
    }
}

With that foundation in place the desired implementation was designed as follows:

public class ScheduleType : FlagsValueObject<ScheduleType> {
    public static readonly ScheduleType Fixed = new FixedType();
    public static readonly ScheduleType Flexible = new FlexibleType();
    public static readonly ScheduleType FullTime = new FullTimeType();
    public static readonly ScheduleType PartTime = new PartTimeType();
    public static readonly ScheduleType Rotated = new RotatedType();

    protected ScheduleType(int value, string name)
        : base(value, name) {
    }

    private ScheduleType(ScheduleType a, ScheduleType b) {
        foreach (var kvp in a.Types.Union(b.Types)) {
            Types[kvp.Key] = kvp.Value;
        }
    }

    public override string Name {
        get {
            return string.Join(", ", Types.OrderBy(_ => _.Value).Select(_ => _.Value)) + " Work Schedule";
        }
    }

    public override ScheduleType And(ScheduleType other) {
        return new ScheduleType(this, other);
    }

    #region Values

    private class FixedType : ScheduleType {
        public FixedType()
            : base(0x01, "Fixed") {
        }
    }

    private class FlexibleType : ScheduleType {
        public FlexibleType()
            : base(0x02, "Flexible") {
        }
    }

    private class FullTimeType : ScheduleType {
        public FullTimeType()
            : base(0x04, "Full Time") {
        }
    }

    private class PartTimeType : ScheduleType {
        public PartTimeType()
            : base(0x08, "Part Time") {
        }
    }

    private class RotatedType : ScheduleType {
        public RotatedType()
            : base(0x10, "Rotated") {
        }
    }
    #endregion
}

Note the hex values used for the concrete implementations.

With a few simple unit tests to show usability:

[TestClass]
public class ScheduleTypeValueObjectTests {
    [TestMethod]
    public void Should_Merge_DisplayNames() {
        //Arrange
        var fixedSchedult = ScheduleType.Fixed; //Fixed Work Schedule
        var fullTime = ScheduleType.FullTime; // Full Time Work Schedule
        var type = fixedSchedult.And(fullTime);

        //Act
        var actual = type.Name;

        //Assert
        actual.Should().Be("Fixed, Full Time Work Schedule");
    }

    [TestMethod]
    public void Should_Check_HasFlag() {
        //Arrange
        var fixedSchedult = ScheduleType.Fixed; //Fixed Work Schedule
        var fullTime = ScheduleType.FullTime; // Full Time Work Schedule
        var partTime = ScheduleType.PartTime;
        var value = fixedSchedult.And(fullTime).And(partTime);

        //Act
        var actual = value.HasFlag(fullTime.And(partTime));

        //Assert
        actual.Should().Be(true);
    }
}

The question I was answering was implying that there are some business rules associated with the possible combinations of values that can be used together in one schedule type.

I am torn about placing such concerns within the concrete implementation and believe that they should be applied within the business layer.

The HasFlag in my opinion allows for that kind of validation to be applied as needed, which was demonstrated in the second example unit test above. For example if a ScheduleType is not allowed to be both FullTime and PartTime it should be a simple matter of checking that within the value object.

Thoughts on current design choices and suggestions on changes if any are welcome.

Answer 1

EnumValueObject

To make it complete, I'd implement IEquatable<EnumValueObject> and add the equality operators == and !=.

Doing it this way will allow us to simplify the equality check.

public abstract class EnumValueObject : IEquatable<EnumValueObject>
{
    public abstract string Name { get; }

    public abstract int Value { get; }

    public override string ToString()
    {
        return Name;
    }

    public bool Equals(EnumValueObject other)
    {
        if (ReferenceEquals(null, other)) return false;
        if (ReferenceEquals(this, other)) return true;
        return Value.Equals(other.Value);
    }

    public override bool Equals(object obj)
    {
        return obj is EnumValueObject value && Equals(value);
    }

    public override int GetHashCode()
    {
        return string.Join(",", Value, Name).GetHashCode();
    }

    public static bool operator ==(EnumValueObject left, EnumValueObject right)
    {
        return Equals(left, right);
    }

    public static bool operator !=(EnumValueObject left, EnumValueObject right)
    {
        return !Equals(left, right);
    }
}

---

FlagsValueObject<>

I would expect to see the bitwise operator | instead of the lengthy And. Implementing it would be simple, I would make And serve as a Template Method and override the operator in the abstract base class:

protected abstract T And(T other);

public static T operator |(FlagsValueObject<T> left, T right)
{
    return left.And(right);
}

ScheduleType can keep it's implementation.

---

Types should be readonly as you're using it in the GetHashCode() method.

---

return string.Join(", ", Types.OrderBy(_ => _.Value).Select(_ => _.Value));

The underscore is usually used to indicate to the reader that the parameter is being ignored, you're clearly not ignoring it in this case, you should rename it something else, let's say t or type.

---

Both Value and Name, should be using some caching mechanism, there is no need to calculate stuff every time. Perhaps version based, like how the dictionary works internally.

Also it seems like SortedDictionary would be a better data structure for your scenario.

---

ScheduleType

This class suffers from some of the problems listed above.

All of those private classes, seem unnecessary and not well-thought-out. They are nothing more than instances of ScheduleType, you already have the base. But instead of creating couple instances, you have to go through all the trouble of declaring new class every time you want to add a new value.

Answer 2

There are few more things that you should consider to change.

---

public override int GetHashCode() {
    return string.Join(",", Value, Name).GetHashCode();
}

Since Value is the actual value of an enum, and the string representaiton is just a friendly-name for it, I'd just use Value.GetHashCode(). Otherwise you have to be careful about the lower/upper case of the Name that would change its hashcode.

---

public override int Value {
    get { return Types.Keys.Aggregate((a, b) => a + b); }
}

There are way too many as and bs everywhere. They should be replaced by stronger names.

---

public override int GetHashCode() {
    return Types.GetHashCode();
}

To me it looks like this should return the hashcode of the keys and not of the dictionary. The base class is basically doing this (or will ;-)) so the behaviour isn't consistent between them.

public override int GetHashCode() {
    return string.Join(",", Value, Name).GetHashCode();
}

Another problem would arise from two instances of the same enum. Their internal dictionaries would return different hashcodes but their keys might be the same so it'll tell you they are different where in reality they aren't.

---

public static readonly ScheduleType Fixed = new FixedType();

I wouldn't create private classes. Calling the private constructor here is enough. Or is there any reason you decided to create private classes anyway?

Answer 3

Using the suggestions from the provided answers and comments I settled on the following.

EnumValuObject implementation was refactored to also include IComparable<T> and the base properties were reverted from being abstract which also means that the value object is immutable once created :

public class EnumValueObject : IEquatable<EnumValueObject>, IComparable<EnumValueObject> {

    protected EnumValueObject(int value, string name) {
        Value = value;
        Name = name;
    }

    protected EnumValueObject() {

    }

    public virtual string Name { get; protected set; }

    public virtual int Value { get; protected set; }

    public static bool operator ==(EnumValueObject left, EnumValueObject right) {
        return Equals(left, right);
    }

    public static bool operator !=(EnumValueObject left, EnumValueObject right) {
        return !Equals(left, right);
    }

    public int CompareTo(EnumValueObject other) {
        return Value.CompareTo(other.Value);
    }

    public bool Equals(EnumValueObject other) {
        if (ReferenceEquals(null, other)) return false;
        if (ReferenceEquals(this, other)) return true;
        return Value.Equals(other.Value);
    }

    public override bool Equals(object obj) {
        return obj is EnumValueObject value && Equals(value);
    }

    public override int GetHashCode() {
        return Value.GetHashCode();
    }

    public override string ToString() {
        return Name;
    }
}

This simplified the FlagsValueObject and removed having to repeatedly calculate the value and name. The bitwise operator was added and the original And was renamed to avoid confusion :

public abstract class FlagsValueObject<T> : EnumValueObject where T : FlagsValueObject<T> {
    protected readonly IDictionary<int, string> Types = new SortedDictionary<int, string>();

    protected FlagsValueObject(int value, string name)
        : base(value, name) {
        Types[value] = name;
    }

    protected FlagsValueObject() {

    }

    public static T operator |(FlagsValueObject<T> left, T right) {
        return left.Or(right);
    }

    protected abstract T Or(T other);

    public virtual bool HasFlag(T flag) {
        return flag != null && (Value & flag.Value) == flag.Value;
    }

    public virtual bool HasFlagValue(int value) {
        return (Value & value) == value;
    }
}

This finally allowed the ScheduleType to have the desired functionality:

public class ScheduleType : FlagsValueObject<ScheduleType> {
    public static readonly ScheduleType Fixed = new ScheduleType(0x01, "Fixed");
    public static readonly ScheduleType Flexible = new ScheduleType(0x02, "Flexible");
    public static readonly ScheduleType FullTime = new ScheduleType(0x04, "Full Time");
    public static readonly ScheduleType PartTime = new ScheduleType(0x08, "Part Time");
    public static readonly ScheduleType Rotated = new ScheduleType(0x10, "Rotated");

    protected ScheduleType(int value, string name)
        : base(value, name) {
    }

    private ScheduleType(ScheduleType a, ScheduleType b) {
        foreach (var kvp in a.Types.Union(b.Types)) {
            Types[kvp.Key] = kvp.Value;
        }
        Name = string.Join(", ", Types.Select(kvp => kvp.Value)) + " Work Schedule";
        Value = Types.Keys.Sum();
    }

    protected override ScheduleType Or(ScheduleType other) {
        var result = new ScheduleType(this, other);

        //Apply validation rules on new combination
        if (result.HasFlag(Fixed) && result.HasFlag(Rotated))
            throw new InvalidOperationException("ScheduleType cannot be both Fixed and Rotated");

        if (result.HasFlag(FullTime) && result.HasFlag(PartTime))
            throw new InvalidOperationException("ScheduleType cannot be both FullTime and PartTime");

        return result;
    }
}

The internal types were removed and created directly as exposed types.

Any business rules related to type combinations can be done when creating the combined types.

Combining types now reflected the proper intent

var fixedSchedule = ScheduleType.Fixed; // Fixed Work Schedule
var fullTime = ScheduleType.FullTime; // Full Time Work Schedule
var partTime = ScheduleType.PartTime; // Part Time Work Schedule

var type = fixedSchedule | fullTime; // Fixed, Full Time Schedule

var hasFullTime = type.HasFlag(fullTime); // Should be true

var value = fullTime | partTime; // Should fail!. InvalidOperation