How to reduce Cyclomatic complexity in an if-else statement

Question

What will you do in this case to reduce the Cyclomatic Complexty

if (Name.Text == string.Empty)
    Name.Background = Brushes.LightSteelBlue;

else if(Age.Text == string.Empty)
    Age.Background = Brushes.LightSteelBlue;

else if(...)
    ...

else
{
    // TODO - something else
}

Let suppose I have 30 or more.

I think reducing cyclomatic complexity should not be a goal by itself. Writing readable code should. — svick, Jun 26 '12 at 13:39
Are Name, Age, etc, polymorphic, or do they share a useful base class? — GregRos, Jun 26 '12 at 13:40
Are you sure this is doing what you want it to? Do you want only one background color to be set? Or do you want the background for _every_ item to be set whether it is empty? — NominSim, Jun 26 '12 at 13:40
@NominSim this is just an example I'm just asking the question to know how others are doing their code. — Wassim AZIRAR, Jun 26 '12 at 13:42
I second NominSim. Highlighting one field at a time will drive the user nuts if there are several bad fields to begin with. — André Chalella, Jun 26 '12 at 13:43
That makes it pretty difficult to modify to reduce the complexity, because what this is saying is that your "nth" background will only be set when all 29 previous text objects are empty. The only way to do this would be to check all previous text objects in some way. — NominSim, Jun 26 '12 at 13:44

Jon Senchyna · Accepted Answer · 2012-06-26T13:50:31.970

3

It looks like you perform the same logic on each "TextBox" (at least I think they are TextBoxes). I would recommend putting all of them into a collection and performing the following logic:

// Using var, since I don't know what class Name and Age actually are
// I am assuming that they are most likely actually the same class
// and at least share a base class with .Text and .BackGround
foreach(var textBox in textBoxes)
{
    // Could use textBox.Text.Length > 0 here as well for performance
    if(textBox.Text == string.Empty)
    {
        textBox.Background = Brushes.LightSteelBlue;
    }
}

Note: This does change your code a bit, as I noticed you only check the value of one "TextBox" only if the previous ones did not have empty text. If you want to keep this logic, just put a break; statement after textBox.Background = Brushes.LightSteelBlue; and only the first empty "TextBox" will have its background color set.

edited Jun 26 '12 at 13:50

answered Jun 26 '12 at 13:41

Jon Senchyna

7,867
2
26
46

This doesn't perform the same functionality that the OPs code does. – NominSim Jun 26 '12 at 13:42
@NominSim It would if you added a `break` inside the `if`. – svick Jun 26 '12 at 13:42
@svick It would be close, you would have to add some sort of else clause to catch the last case... I'm going to switch my -1 anyways though since the OP has now specified it was just an example. – NominSim Jun 26 '12 at 13:45
I noticed that as soon as I posted the code. I was in mid-edit when you commented. I'm leaving it as is, as I imagine the OP wouldn't actually want to only highlight one "TextBox" at a time, but I added a note at the bottom stating the suggested fix if they actually do want that. – Jon Senchyna Jun 26 '12 at 13:48

score 2 · Answer 2 · answered Jun 26 '12 at 13:42

2

For example for this concrete case, you can

define a Dictionary<string, dynamic> dic where KEY is a string-value, and VALUE is dynamic(Name, Age...whatever)
do dic[stringValue].Background = Color.LightSteelBlue;

Just an example.

You may want to choose dynamic or not. May be something more intuitive and easy to understand, but the basic idea is:

make use of dictionary with key based on if's right-value and like a value some operation/method/object.

Hope this helps.

answered Jun 26 '12 at 13:42

Tigran

61,654
8
86
123

1

Everytime someone uses ``dynamic`` not because of interop, but because he wants to write code like in a dynamic language, a kitten dies somewhere ;) Seriously, why use a strongly typed language if you want to do dynamic stuff? I somehow dont like it... – Philip Daubmeier Jun 26 '12 at 13:49
@PhilipDaubmeier: it's like open your wings and jump from the hill down. If one doesn't know to fly, just do not jump. In fact, I explained *basic* idea behind the solution and give some custom implementation, that *may*, or *may not* be choosen. – Tigran Jun 26 '12 at 13:53
yes, I get your point. I just wanted to point out that people should think about whether they really need to before using ``dynamic``. – Philip Daubmeier Jun 26 '12 at 13:57

Andras Zoltan · Answer 3 · 2012-06-26T13:57:25.300

I agree with svick's comment completely. In some cases, the following approach can be good (but not to reduce cyclomatic complexity, generally to create pluggable decision-makers):

public class SwitchAction{
  public Func<bool> Predicate { get; set; }
  public Action TheAction { get; set; }
}

public List<SwitchAction> SwitchableActions = new List<SwitchAction>();

public void InitialiseSwitchableActions()
{
   SwitchableActions.AddRange(new[] {
     new SwitchAction() { Predicate = () => Name.Text == string.Empty, 
                          TheAction = () => Name.Background = Brushes.LightSteelBlue },
     new SwitchAction() { Predicate = () => Age.Text == string.Empty, 
                          TheAction = () => Age.Background = Brushes.LightSteelBlue },
   });
}

public void RunSwitchables()
{
  var switched = SwitchableActions.FirstOrDefault(s => Predicate());

  if(switched != null)
    switched.TheAction();
  else
    //TODO: something else.
}

Of course - if actually these actions are not mutually exclusive you have to change that last method a little bit:

public void RunSwitchables()
{
   bool runCatchAll = true;
   foreach(var switched in SwitchableActions.Where(a => a.Predicate())
   {
     switched.TheAction();
     runCatchAll = false;
   }

   if(runCatchAll)
     //TODO: Something else.
}

Are either of these more readable though? Hmm... probably not.

How to reduce Cyclomatic complexity in an if-else statement

3 Answers3