CODEWITHSUNDEEP
c#cyclomatic-complexity
Farmer
Farmer

Reputation: 10983

How to reduce Cyclomatic complexity in an if-else statement

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.

Upvotes: 3

Views: 4706

Answers (3)

Andras Zoltan
Andras Zoltan

Reputation: 42333

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.

Upvotes: 0

Jon Senchyna
Jon Senchyna

Reputation: 8037

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.

Upvotes: 3

Tigran
Tigran

Reputation: 62248

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.

Upvotes: 2

Related Questions