8
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I'm a little confused by your use of the term Value Object in this context. I don't think you mean Value Object from DDD, or Value Type (as in, a struct). I would consider a different name to avoid potential confusion. \$\endgroup\$ – Aaron M. Eshbach May 2 '18 at 17:31
  • \$\begingroup\$ In other words, you want to make it impossible to even create invalid combinations, which is why just writing a few business-level methods like bool IsValidCombination(ScheduleTypes) or ScheduleTypes[] GetValidCombinations() won't suffice? What if the business rules change, and you have to migrate old data? \$\endgroup\$ – Pieter Witvoet May 3 '18 at 7:47
  • \$\begingroup\$ @PieterWitvoet I think I an starting to see what you mean. I am thinking that validation can be done when combining them. \$\endgroup\$ – Nkosi May 3 '18 at 9:32
  • 1
    \$\begingroup\$ The main advantage over a simple enum is that this allows you to prevent invalid combinations (such that MyEnum.A | MyEnum.NotA would throw an exception). The benefit is obvious: you don't need to check validity anywhere else. But that approach does not work well if validity depends on context (not all companies allowing the same working schedules, or an older software version allowing different combinations). And if you decide to do validation elsewhere, in some business-layer code, then what's the point of using this custom enum type instead of a simple enum? \$\endgroup\$ – Pieter Witvoet May 3 '18 at 10:32
  • \$\begingroup\$ @PieterWitvoet that is what I eventually realized by your comment. You provided the light-bulb moment for me. ;) \$\endgroup\$ – Nkosi May 3 '18 at 10:38
7
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I originally had the bitwise operator but thought I was over engineering it and refactored it out.. I totally get your point about it though. checking out the other suggestions. \$\endgroup\$ – Nkosi May 2 '18 at 15:09
  • \$\begingroup\$ @Nkosi It seems more intuitive to me and it also shortens the syntax, so I think it's a good addition. It doesn't take much code to implement either. \$\endgroup\$ – Denis May 2 '18 at 15:10
  • 1
    \$\begingroup\$ It's nice to see the is operator in action ;-] \$\endgroup\$ – t3chb0t May 2 '18 at 16:50
  • 1
    \$\begingroup\$ Why are you using the OR operator to call the AND function? \$\endgroup\$ – Arturo Torres Sánchez May 2 '18 at 16:54
  • 3
    \$\begingroup\$ @TopinFrassi the value is in the first condition. newer c# syntax. If obj is of type EnumValuObject then assign it to value. \$\endgroup\$ – Nkosi May 2 '18 at 17:41
4
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ for the private types that came over from the OP of the question I was answering. plus I saw it in an article. here makes sense to call the private constructor. \$\endgroup\$ – Nkosi May 2 '18 at 16:51
4
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ return obj is EnumValueObject && Equals((EnumValueObject)obj); you have double cast here. Don't you like the new is operator? ;-) \$\endgroup\$ – t3chb0t May 3 '18 at 10:54
  • \$\begingroup\$ @t3chb0t copied directly from current IDE. I have not upgraded as yet. ;). will fix in answer. :P \$\endgroup\$ – Nkosi May 3 '18 at 10:57
  • \$\begingroup\$ Looks a lot cleaner :) \$\endgroup\$ – Denis May 3 '18 at 14:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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