Return the state of collection based on individual object states

Question

The following code is a simplified version of what I want to achieve. Each object may have one of several states, and the overall collection state depends on the state of each of these objects. Currently, the code is written in the form of several if-else blocks, however, it seems ugly and non-extensible. By that, I mean, tomorrow if additional object states are added, I would end up adding several more flags and if-conditions, making the code difficult to read and maintain. Can this be done in a better way?

internal enum State
{
    Error,
    Warning,
    Okay
}

internal enum CollectionState
{
    Error,
    AllWarnings,
    AllOkay,
    OkayAndWarnings
}

internal class MyClass
{
    //other properties...

    internal State State { get; set; }
}

internal class MyBusinessLogic
{
    /// <summary>
    /// 1. If any object is in Error state, CollectionState is Error.
    /// 2. If all objects are in Warning state, CollectionState is AllWarnings.
    /// 3. If all objects are in Okay state, CollectionState is AllOkay.
    /// 4. If some objects are in Okay state and some in Warning state, CollectionState is OkayAndWarnings
    /// </summary>
    internal CollectionState GetCollectionState(IList<MyClass> objectList)
    {
        bool isError, isOkay, isWarning;
        isError = isOkay = isWarning = false;
        foreach (var myObject in objectList)
        {
            if (myObject.State == State.Error)
            {
                isError = true;
                break;
            }
            if(myObject.State == State.Okay)
            {
                isOkay = true;
            }
            else if (myObject.State == State.Warning)
            {
                isWarning = true;
            }
        }

        if (isError)
        {
            return CollectionState.Error;
        }
        else if(!isOkay && isWarning)
        {
            return CollectionState.AllWarnings;
        }
        else if (isOkay && !isWarning)
        {
            return CollectionState.AllOkay;
        }
        else //if(isOkay && isWarning)
        {
            return CollectionState.OkayAndWarnings;
        }
    }
}

Answer 1

Quick feedback

and the overall collection state depends on the state of each of these objects

The criteria for each collection state (based on the collection's element's states) has to be defined somewhere. You can't get around that.

Currently, the code is written in the form of several if-else blocks, however, it seems ugly and non-extensible. By that, I mean, tomorrow if additional object states are added, I would end up adding several more flags and if-conditions, making the code difficult to read and maintain.

It can be cleaned up (I added a suggestion below). This improves the readability and maintainability.

However, it's important to understand that while you can avoid the if chain in and of itself, you cannot avoid having to define the criteria.

isOkay , isWarning , isError

These names are not fitting of the variable. Especially since all three could be true at the same time. A more fitting name would be containsError (and similar).

---

Reviewing your code

internal enum CollectionState
{
    Error,
    AllWarnings,
    AllOkay,
    OkayAndWarnings
}

An enum, at least by itself, is not a good solution here. Each member of the enum has its own criteria for being valid or not. However, the criteria are not linked to the value themselves, which makes it less than ideal to find out what the criteria of a given state are.

if (isError)
{
    return CollectionState.Error;
}
else if(!isOkay && isWarning)
{
    return CollectionState.AllWarnings;
}
else if (isOkay && !isWarning)
{
    return CollectionState.AllOkay;
}
else //if(isOkay && isWarning)
{
    return CollectionState.OkayAndWarnings;
}

Here, you've listed the criteria. You've already identified that the if chain is an ugly solution; which it is.

It's important to notice that the combined criteria of all collection states should cover all cases but also have no overlap.

As to covering all cases; you have no guarantee that this is being done correctly. You're ending the chain on an else, which means that anything that hasn't matched yet will be given this default collectionstate. This might be what you want, but I can see a similar argument that you want to avoid this and would rather be alerted of not finding an explicit match (it helps with future maintainability).

As to preventing overlap; that is currently the case. But the way it's implemented now leaves gaps as to enforcing this behavior. If an issue arises where two criteria can have overlap (e.g. NoErrors and AllWarnings), that may be initially hidden because the if chain will return the first found match and ignore the second. If the chain gets reshuffled at some point, you're suddenly going to be faced with unexpected behavior (as the second criteria may nog be returned as the first found match).

So I would like to focus on improving these things:

• A clear connection between a collection state and its criteria.
• Minimizing the needed code footprint for adding new statecollections
• Alerting developers of overlapping criteria (which is a logical problem)
• Optional - Alerting developers of a situation which yields no matches (which can be a problem)

---

My proposed solution

• A clear connection between a collection state and its criteria.

public class CollectionStateDefinition
{
    public CollectionState State { get; set; }
    public Func<IEnumerable<MyClass>, bool> Criteria { get; set; }

    public CollectionStateDefinition(CollectionState state, Func<IEnumerable<MyClass>, bool> criteria)
    {
         this.State = state;
         this.Criteria = criteria;
    }
}

public static List<CollectionStateDefinition> CollectionStateDefinitions = new List<CollectionStateDefinition>()
{
    new CollectionStateDefinition(CollectionState.Error, 
            coll => coll.Any(myObject => myObject.State == State.Error)),
    new CollectionStateDefinition(CollectionState.AllWarnings,
            coll => coll.All(myObject => myObject.State == State.Warning)),
    new CollectionStateDefinition(CollectionState.AllOkay,
            coll => coll.All(myObject => myObject.State == State.Okay))
}

This creates a clear mapping between a collection state and the criteria for matching the collection state.

I omitted OkayAndWarnings from the first example, because this is a bit of a special case:

new CollectionStateDefinition(CollectionState.OkayAndWarnings, 
            coll => 
                 !coll.Any(myObject => myObject.State == State.Error)
                 && coll.Any(myObject => myObject.State == State.Warning)
                 && coll.Any(myObject => myObject.State == State.Okay)
)

These criteria are only met if:

• No errors are present
• At least one Warning exists
• At least one Okay exists

Another mention here is that you could've used a different approach here, but I specifically chose not to:

• Using tuples instead of CollectionStateDefinition. This would work and would preclude the need for a custom built class.
  • I don't like unnamed tuples, as they hinder readability.
  • Named tuples would be more readably. There's nothing wrong with taking this approach, I simply prefer building an explicit class. Especially since you're already considering future extensability.
• I get the feeling that some people would err towards a Dictionary<CollectionState,Func<..>> here. While I can see why (because every CollectionState has one criteria function), I wouldn't use it here, because this would force us to find the key by looking for the value, which is the opposite of how a Dictionary is supposed to work. It's possible to do so on a technical level, but it creates unnecessarily contrived code to do so.

• Minimizing the needed code footprint for adding new statecollections

This is achieved here. All you need to do is add an extra StateCollectionDefinition to the list.

I haven't shown the method that looks for matching criteria yet, but it will not require any changes when a new entry is added to the list.

• Alerting developers of overlapping criteria (which is a logical problem)

public static CollectionState GetCollectionState(IEnumerable<MyClass> myList)
{
    var matchingCriteria = CollectionStateDefinitions.Where(csd => csd.Criteria.Invoke(myList));

    if(!matchingCriteria.Any())
        throw new Exception("There is no matching state collection!");

    if(matchingCriteria.Count() > 1)
        throw new Exception("There is more than one matching state collection!");

    return matchingCriteria.Single().State;
}

This is a barebones example. It would be nice if you could say which criteria were matched when you find more than one match. Over all, the exceptions can be improved, but I kept it simple for the sake of example.

• Optional - Alerting developers of a situation which yields no matches (which can be a problem)

This is already achieved in the above method.

If you want to provide a default value instead, you can easily change the code:

if(!matchingCriteria.Any())
    return CollectionState.OkayAndWarnings;

You could also do both, e.g. log a message (to a log file) and return a default value anyway.

---

Footnote

Personally, I wouldn't use OkayAndWarnings. I suspect you've currently implemented OkayAndWarnings specifically because it's supposed to be a default for when the other criteria don't match; rather than OkayAndWarnings being a useful classification in and of itself.

I would tend to use a system similar to:

• Show error state if errors exist.
• Show warning state if warnings exist (and no errors).
• Show okay state if there are no warnings or errors.

This would actually preclude the need for a StateCollection enum. You could reuse your same State enum, with a method that takes a IEnumerable<MyClass> and returns a State, where the logic basically follows the bullet points I just mentioned.

However, in such a case, if is actually a decent enough approach since the example is so simple.
My proposed solution is more complex because it's built to work for any number of states and catches several fringe issues (no or more matches).

Answer 2

You can also use bitwise flags for that:

[Flags]
internal enum State
{
    None = 0,
    Error = 1,
    Warning = 2,
    Okay = 4,

    OkayAndWarning = Okay | Warning,
}

internal State GetCollectionState(IList<MyClass> objectList) 
{
    return objectList.Select(o => o.State)
                     .Aggregate((result, next) => result |= next);
}

You can then check the aggregated state using HasFlag method or equality operator:

var state = GetCollectionState(...);
var error = state.HasFlag(State.Error);
var allWarnings = state == State.Warning;
var allOkay = state == State.Okay;
var okayAndWarnings = state == State.OkayAndWarning;
//etc.

On the plus side this approach is easy to implement and State enum is easy to extend when needed. Combining flags is a trivial process of applying | (bitwise OR) operator.

On the minus side you give up some of the type safety. With this approach any flag combination is a valid state, (including no flags at all). Also there is no longer a distinction between individual state and collection state (both use State enum), compiler will not warn you if you accidentally mix them. So in general, you'll have to be more careful, when checking states.

Answer 3

A solution to your problem might be to make the State composable. Then, you can use System.Linq.Enumerable.Aggregate() to compute the collection state. Note that in fact you have 2 different states, such that the "space" of collection state is larger than the one for object state.

Think of State as capable of being operated on. For example:

collState = collState + objState;

For simple enums implementing an operation might be overkill. There is an Aggregate<TSource, TAccumulate>() version taking Func<TAccumulate, TSource, TAccumulate> func, where you could implemente the state-composition operation. You may implement it with array lookup.

On the other hand, if what you really need is just a summary of error conditions, then the other answers (such as using .All(), .Any(), etc.) might have more appropriate solutions.

Answer 4

I would collect the states into a set, then check for the 'severe' state, otherwise check for size (this will be probably syntactically wrong because C# in not my primary language):

ISet<State> states = new HashSet<State>(objectList.Select(o => o.State));

if (states.Overlaps(CollectionState.ErrorStates) {
  states.IntersectWith(CollectionState.ErrorStates);
}
if (states.Count == 1) {
  return CollectionState.MapObjectState(states.First);
}
return CollectionState.Mixed;

You can then define your "normal" single state mapping and extend it as needed with new states, without touching the aggregation at all.

If you ever need a specific mapping for different types of mixed states, you only have to change the last line to take them into account.

Answer 5

Pattern matching in switches can be helpful. Used with Tuples and Local Functions your code can be very clear.

I use three parts.

1) Create a Tuple that encapsulates relevant information. You pre-process/group conditions that apply to all states. You can give the conditions meaningful names. If you can, convert to bools.

2) Create Local Functions for each output condition. By using locals you don't have to pass anything.

3) Switch on the tuple using Pattern Matching.

If you can reduce conditions to bools you can use Karnaugh Maps to greatly reduce the amount of logic.

For example.

CollectionState Method() {
    // Create tuple with friendly names of relevant information
    (Enum1 en, bool isA, bool hasB) switchState = (
        EnumVariable,
        (pre-process code for state1) ? true :false,
        (pre-process code for state2) ? true : false
    )
    // Switch using patterns
    switch(switchState){
        case var s when (s.en == Enum1.A && s.isA && !s.hasValue): return ProcessConditionA();
        case var s when (s.en == Enum1.B && !s.isA && s.hasValue): return ProcessConditionB();
   }

   // Locals
   CollectionState ProcessCondionA(){..}
   CollectionState ProcessCondionB(){..}
}

Answer 6

Consider a collection class.

It is still messy when you add more states but you can use some tricks from other answers.

I don't like calling a property the same name and the enum.

I like lower case for internal but that may not be the standard.

Don't skip all for OkayAndWarnings.

internal enum States { error, okay, warning, none };
public class MyClass
{
    //other properties 
    internal States State { get; set; }
}
internal enum CollectionState { error, allWarnings, allOkay, allOkayAndWarnings, none }
internal class MyClassCol
{
    internal List<MyClass> MyClasses { get; } = new List<MyClass>();

    internal CollectionState MyClassColState
    {   get 
        {
            bool isOkay, isWarning;
            isOkay = isWarning = false;
            foreach (MyClass myClass in MyClasses)
            {
                if(myClass.State == States.error)
                {
                    return CollectionState.error;
                }
                if(myClass.State == States.okay)
                {
                    isOkay = true;
                }
                if (myClass.State == States.warning)
                {
                    isWarning = true;
                }
            }
            if(isOkay && isWarning)
            {
                return CollectionState.allOkayAndWarnings;
            }
            if (!isOkay && isWarning)
            {
                return CollectionState.allWarnings;
            }
            if (isOkay && !isWarning)
            {
                return CollectionState.allOkay;
            }
            return CollectionState.none;
        }
    }
}