Way(s) to avoid continuous, similar conditional blocks

Question

Want to know if there is a better approach to handle multiple, similar conditional statements and actions such as in the example snippet below.

private void AddCommonDictionaryItemsForAllAttributes(MyCustomType dc, string statusFlag)
{
    if (dc.xmlAttributes == null) {
        dc.xmlAttributes = new Dictionary<string, string>();
    }
    dc.xmlAttributes.Add(Constant.CD_1, statusFlag);
    dc.xmlAttributes.Add(Constant.CD_2, statusFlag);
    dc.xmlAttributes.Add(Constant.CD_3, statusFlag);
    if (dc.primaryZone != null) {
        dc.xmlAttributes.Add(Constant.CD_4, statusFlag);
    }
    if (dc.Mgr1 != null) {
        dc.xmlAttributes.Add(Constant.CD_10, statusFlag);
    }
    if (dc.Mgr2 != null) {
        dc.xmlAttributes.Add(Constant.CD_11, statusFlag);
    }
    if (dc.Mgr3 != null) {
        dc.xmlAttributes.Add(Constant.CD_5, statusFlag);
    }    
    if (dc.Producer != null) {
        dc.xmlAttributes.Add(Constant.CD_6, statusFlag);
    }
    if (dc.CountTest > 0) {
        dc.xmlAttributes.Add(Constant.CD_7, statusFlag);
    }
    if (dc.List1 != null && dc.List1.Count > 0) {
        dc.xmlAttributes.Add(Constant.CD_8, statusFlag);
    }
    if (dc.List2 != null && dc.List2.Count > 0) {
        dc.xmlAttributes.Add(Constant.CD_9, statusFlag);
    }
}

The if conditions and the addition to the dictionary operation seems to me as redundant so looking out for more efficient and elegant ways to code this.

Thanks!

Update: I am using .NET 3.5

to reduce space you could put the statements on the same lines as the conditions, but i cant see any way to get rid of the long list of conditions. sorry. — Darkhydro, Feb 15 '11 at 07:37

jevakallio · Accepted Answer · 2011-02-15T13:20:29.740

You could create a helper type, which provides a test to be performed on an instance of MyCustomType, and the key to use in the xmlAttributes dictionary:

class Rule
{
    private readonly Predicate<MyCustomType> _test;
    private readonly string _key;

    public Predicate<MyCustomType> Test { get { return _test; } }
    public string Key { get { return _key;  } }

    public Rule(Predicate<MyCustomType> test, string key)
    {
        _test = test;
        _key = key;
    }
}

You can then create a set of these rules, and enumerate them:

    private void AddCommonDictionaryItemsForAllAttributes(MyCustomType dc, string statusFlag)
    {

        var rules = new Rule[]
        {
            new Rule(x => x.Mgr1 != null, Constant.CD_4),
            new Rule(x => x.Mgr2 != null, Constant.CD_10),
            //...snip...
            new Rule(x => x.List2 != null && x.List2.Count > 0, Constant.CD_9)
        };

        foreach(var rule in rules.Where(r => r.Test(dc)))
            dc.xmlAttributes.Add(rule.Key, statusFlag);
    }

Beaten by Jon Skeet. What's the point of even trying to answer C# questions in the GMT daytime :) — jevakallio, Feb 15 '11 at 07:53
Using a custom type for this is fine, but I'd suggest making it immutable, and using properties instead of public fields. Whether it's a class or a struct doesn't matter too much though IMO. — Jon Skeet, Feb 15 '11 at 12:47
@Jon Skeet, valid points. Changed my code sample to not impart bad habits onwards. — jevakallio, Feb 15 '11 at 13:21

score 2 · Answer 2 · edited Feb 15 '11 at 13:36

One option would be to have some sort of list of conditions and the constants represented by those conditions. For example:

var list = new List<Tuple<Predicate<MyCustomType>, string>>
{
    Tuple.Create(dc => true, Constant.CD_1),
    Tuple.Create(dc => true, Constant.CD_2),
    Tuple.Create(dc => true, Constant.CD_3),
    Tuple.Create(dc => dc.primaryZone != null, Constant.CD_4),
    Tuple.Create(dc => dc.Mgr1 != null, Constant.CD_5),
    // etc
};

Then you could just iterate over the list, setting the relevant to status in the dictionary whenever the predicate was true:

foreach (var tuple in list)
{
    if (tuple.Item1(dc))
    {
        dc.xmlAttributes.Add(tuple.Item2, statusFlag);
    }
}

Note that you can set the list up statically once and then reuse it everywhere, as the list itself doesn't change.

Thanks but I am working on .NET 3.5. :( What would be a suitable alternative of Tuple? — Dienekes, Feb 15 '11 at 12:35
@Dienekes: You can write your own equivalent of Tuple very easily, or a custom type as per fencliff's answer - although I wouldn't use a mutable struct. — Jon Skeet, Feb 15 '11 at 12:47

score 1 · Answer 3 · edited May 23 '17 at 12:13

1

You can do it in for loop, because you can cast into to enum.

edited May 23 '17 at 12:13

Community

1
1

answered Feb 15 '11 at 07:36

Matěj Zábský

16,909
15
69
114

score 0 · Answer 4 · answered Feb 15 '11 at 09:10

Consider encapsulating attributes collection inside YourCustomClass. This will protect your attributes from accidental changing, and it will move attributes filling logic toward the data it belongs to.

Benefits:

You can change attributes filling implementation anytime, without changing clients (conditions, predicates collection etc).
Much more clean client
Easier maintenance

So, even with your default implementation usage will look like this:

dc.SetStaus(string statusFlag)

And all the dirty job will be done inside dc (BTW I advice to use enum CD instead of constants, but it's up to you):

public void SetStatus(string statusFlag)
{
    if (_xmlAttributes == null)
        _xmlAttributes = new Dictionary<CD, string>();

    _xmlAttributes.Add(CD.CD_1, statusFlag);
    _xmlAttributes.Add(CD.CD_2, statusFlag);
    _xmlAttributes.Add(CD.CD_3, statusFlag);

    if (_primaryZone != null)
        _xmlAttributes.Add(CD.CD_4, statusFlag);

    if (_mgr1 != null)
        _xmlAttributes.Add(CD.CD_10, statusFlag);

    if (_mgr2 != null)
        _xmlAttributes.Add(CD.CD_11, statusFlag);

    if (_mgr3 != null)
        _xmlAttributes.Add(CD.CD_5, statusFlag);

    if (_producer != null)
        _xmlAttributes.Add(CD.CD_6, statusFlag);

    if (_countTest > 0)
        _xmlAttributes.Add(CD.CD_7, statusFlag);

    if (_list1 != null && _list1.Count > 0)
        _xmlAttributes.Add(CD.CD_8, statusFlag);

    if (_list2 != null && _list2.Count > 0)
        _xmlAttributes.Add(CD.CD_9, statusFlag);
}

After that you can refactor implementation easily:

private Dictionary<CD, Func<bool>> _statusSetConditions;

public MyCustomType()
{
    _statusSetConditions = new Dictionary<CD, Func<bool>>();
    _statusSetConditions.Add(CD.CD_1, () => true);
    _statusSetConditions.Add(CD.CD_2, () => true);
    _statusSetConditions.Add(CD.CD_3, () => true);
    _statusSetConditions.Add(CD.CD_4, () => _primaryZone != null);
    ...
    _statusSetConditions.Add(CD.CD_11, () => _mgr2 != null);
}

public void SetStatus(string statusFlag)
{
    if (_xmlAttributes == null)
        _xmlAttributes = new Dictionary<CD, string>();

    foreach (CD cd in Enum.GetValues(typeof(CD)))
        AddStatusAttribute(cd, statusFlag);
}

private void AddStatusAttribute(CD cd, string statusFlag)
{
    Func<bool> condition;

    if (!_statusSetConditions.TryGetValue(cd, out condition))
        return; // or throw exception

    if (condition())
        _xmlAttributes.Add(cd, statusFlag);
}

And client still just calls to dc.SetStatus(statusFlag);

Maybe after encapsulating this status-set logic, you will just save status in field of YourCustomClass.

score 0 · Answer 5 · answered Feb 16 '11 at 07:17

Meh. It's not really redundant, unless you want to consider something like java reflection. Consider helper methods:

void addIfOk(int test, MyCustomType dc, String attr, string statusFlag) {
  if(test!=0) dc.xmlAttributes.Add(attr, statusFlag);
}
void addIfOk(Object test, MyCustomType dc, String attr, string statusFlag) {
  if(test!=null) dc.xmlAttributes.Add(attr, statusFlag);
}
void addIfOk(Collection test, MyCustomType dc, String attr, string statusFlag) {
  if(test!=null&&!test.isEmpty()) dc.xmlAttributes.Add(attr, statusFlag);
}

The code then becomes:

    addIfOk(dc.Mgr1, dc, Constant.CD_10, statusFlag);
    addIfOk(dc.Mgr2, dc, Constant.CD_11, statusFlag);
    addIfOk(dc.Mgr3, dc, Constant.CD_5, statusFlag);
    addIfOk(dc.Producer, dc, Constant.CD_5, statusFlag);

and so on. Perhaps this would make more sense as a method inside your custom type: setXmlStatusAttributes(statusfFlag)

score -1 · Answer 6 · answered Feb 15 '11 at 07:38

-1

There are two ways: 1. Use switch case 2. Use Ternary operator

both will make your code to look clean, however in your case switch case doesn't work.

answered Feb 15 '11 at 07:38

Ravi Vanapalli

9,805
3
33
43

Using switch case will only work if always only one case will apply. I don't think this is the case with this code. I also don't see how the ternary operator will help here. – codymanix Feb 15 '11 at 08:14

Way(s) to avoid continuous, similar conditional blocks

6 Answers6