6
\$\begingroup\$

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;
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ I'm voting to close this question as off-topic because OP posted a simplified version of the code. \$\endgroup\$ – t3chb0t May 7 '18 at 10:10
  • 8
    \$\begingroup\$ As a counter to @t3chb0t, I would distinguish between simplified logic and code here. Simplified logic does indeed defeat the purpose of reviewing the code. But if the code sample was redacted by simply reducing a large amount of enum values to four values, or redacting unused properties of a class, nothing is lost by doing so, and the reason for closing doesn't apply (imo). If anything, I appreciate that OP effectively redacted MyClass to the MCVE variant, as his question isn't "review all my code" but rather asking for a review on a particular approach/algorithm. \$\endgroup\$ – Flater May 7 '18 at 10:18
  • \$\begingroup\$ @t3chb0t as correctly interpreted by @ Flater, the logic for this particular operation is intact and is not simplified (even the enum states currently in use is just 4, I'd mentioned additional enum states to understand a solution which would be extensible). I haven't mentioned other properties and methods which are irrelevant for this particular operation. I hope this doesn't count as "a simplified version of the logic" and doesn't classify the question as off-topic. \$\endgroup\$ – Fahad May 7 '18 at 11:21
  • 1
    \$\begingroup\$ The fact that they have very "example code-y" class names is probably attracting extra close votes as well. We do also explicitly allow for reviewing only a subset of a codebase, assuming that we have enough context to review the pieces present. Given that, I'm voting to leave open. \$\endgroup\$ – Dannnno May 7 '18 at 13:44
  • 1
    \$\begingroup\$ @Fahad This doesn’t explain how you’re using the state, or why the state can’t be represented by the objects themselves. For some explanation, check the Tell, don’t ask software design principle. \$\endgroup\$ – Konrad Rudolph May 7 '18 at 15:58
7
\$\begingroup\$

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).

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Reviewing simplfied code is usually counterproductive... \$\endgroup\$ – t3chb0t May 7 '18 at 10:11
  • 1
    \$\begingroup\$ @t3chb0t: I see no meaningful difference between OP posting an example with 4 enum states or one with 25 enum states. It's the same principle at play. Now if OP has used (over)simplified logic, I agree with you (since the logic is what's being reviewed). But the the logic is well explained and is not lacking anything as far as I can see. \$\endgroup\$ – Flater May 7 '18 at 10:15
  • 2
    \$\begingroup\$ @Flater thank you for the detailed explanation and additional tips. Love the bit about A clear connection between a collection state and its criteria., I hadn't thought about it but now it makes a lot of sense to couple the state and the corresponding criteria. I think I'm gonna go this way. :) \$\endgroup\$ – Fahad May 7 '18 at 11:49
  • \$\begingroup\$ @Flater I have a doubt though. var matchingCriteria = CollectionStateDefinitions.Where(csd => csd.Criteria.Invoke(myList)); Wouldn't this lead to performance degradation? My code iterates over the collection once and gives the answer, however, this would iterate over the collection n times, where n is the number of collection states. \$\endgroup\$ – Fahad May 7 '18 at 12:50
  • 2
    \$\begingroup\$ @Fahad: Yes, but do also note that Any is shortcircuited. Once it finds the first occurence, it returns true without evaluating further. You're right about a minor performance gain, but you need to observe the needed code to achieve a unified iteration. The impact on the code readability outweighs the performance benefit, in my opinion. Unless you're talking about massive amounts of data and a strong focus on performance (well beyond common expectations). \$\endgroup\$ – Flater May 7 '18 at 12:53
10
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ This is a good idea, but instead of having the Flags-attribute on State, I would use it on CollectionState which then should have names like "HasErrors", "HasWarnings" with same numeric values as State.Error, State.Warning etc. and collect the collectionstate much the same way as you do . If Statehas flags it is a potential source to errors and misunderstandings. \$\endgroup\$ – Henrik Hansen May 7 '18 at 11:51
  • \$\begingroup\$ Thanks, this seems like a really good idea too. I guess we would still have several checks to identify the collection state, albeit simplified. :) \$\endgroup\$ – Fahad May 7 '18 at 15:38
0
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

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(){..}
}
| improve this answer | |
\$\endgroup\$
-2
\$\begingroup\$

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;
        }
    }
}
| improve this answer | |
\$\endgroup\$

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.