Decreasing the Cyclomatic Complexity without affecting the business logic

Question

consider this method:

public ActionResult DoSomeAction(ViewModel viewModel)
{
    try
    {
        if (!CheckCondition1(viewModel))
            return Json(new {result = "Can not process"});

        if (CheckCondition2(viewModel))
        {
            return Json(new { result = false, moreInfo = "Some info" });
        }

        var studentObject = _helper.GetStudent(viewModel, false);

        if (viewModel.ViewType == UpdateType.AllowAll)
        {
            studentObject = _helper.ReturnstudentObject(viewModel, false);
        }
        else
        {
            studentObject.CourseType = ALLOW_ALL;
            studentObject.StartDate = DateTime.UtcNow.ToShortDateString();
        }

        if (studentObject.CourseType == ALLOW_UPDATES)
        {
            var schedule = GetSchedules();

            if (schedule == null || !schedule.Any())
            {
                return Json(new { result = NO_SCHEDULES });
            }

            _manager.AddSchedule(schedule);
        }
        else
        {
            _manager.AllowAllCourses(studentObject.Id);
        }

        _manager.Upsert(studentObject);

        return Json(new { result = true });
    }
    catch (Exception e)
    {
        // logging code
    }
}

This method has many exit points and has a Cyclomatic Complexity of 8. Our best practices say that it should not be greater than 5.

• Is it because of multiple IFs?

• What can I do to refactor this so that it has less exit points?

Thanks in advance.

Answer 1

---

This is a summary of my comments beneath the question above

---

Our best practices say that it should not be greater than 5.

"5" sounds a bit low. nDepend and Microsoft recommend "30" and "25" respectively.

nDepend:

Methods where CC is higher than 15 are hard to understand and maintain. Methods where CC is higher than 30 are extremely complex and should be split into smaller methods (except if they are automatically generated by a tool)

Microsoft:

cyclomatic complexity = the number of edges - the number of nodes + 1 where a node represents a logic branch point and an edge represents a line between nodes. The rule reports a violation when the cyclomatic complexity is more than 25.

OP:

"Is it because of multiple IFs" -

yes and the ||

What can I do to refactor this so that it has less exit points?

An easy way is to just take the method and "split into smaller methods". i.e. instead of all the ifs in one method, move some of your if logic into one or more methods, each calling the other.

e.g.

class Class1
{
    class  Hobbit
    {
         
    }

    void Foo(object person)
    {
        if (...)
        {
                // ...
            
        }
        else if (...)
        {
                // ...
        }

        if (x == 1 && person is Hobbit)
        {
            if (...)
            {
                // ...
            }

            if (...)
            {
                // ...
            }

            if (...)
            {
                // ...
            }

        }
    }
}

Could be improved by moving the inner-most group of ifs into a separate method:

    void Foo(object person)
    {
        if (...)
        {
                // ...
            
        }
        else if (...)
        {
                // ...
        }

        if (x == 1 && person is Hobbit)
        {
            DoHobbitStuff();
        }
    }

    void DoHobbitStuff()
    {
        if (...)
        {
            // ...
        }

        if (...)
        {
            // ...
        }

        if (...)
        {
            // ...
        }
    }

However, at "5" I don't believe your code requires refactoring for the purpose of CC reduction.

What can I do to refactor this so that it has less exit points?

According to nDepend, exit points such as return are not counted:

Following expressions are not counted for CC computation:

else | do | switch | try | using | throw | finally | return | object creation | method call | field access

Answer 2

Looking at your code, it is obvious that your high cyclomatic complexity and hard way to refactor are indicators of bad design (eg. code smells). Lets review it a little.

_helper
_manager

What are these things? Why do they have such ambiguous names? If you cannot find any other fitting name than those, it means your separation of concerns is wrong.

_helper.GetStudent(viewModel, false);
_helper.ReturnstudentObject(viewModel, false);

I can't even imagine how can those methods work. How can some generic helper know how to get "student" from generic ViewModel? And what is difference between those two methods?

    var studentObject = _helper.GetStudent(viewModel, false);

    if (viewModel.ViewType == UpdateType.AllowAll)
    {
        studentObject = _helper.ReturnstudentObject(viewModel, false);
    }
    else
    {
        studentObject.CourseType = ALLOW_ALL;
        studentObject.StartDate = DateTime.UtcNow.ToShortDateString();
    }

This really looks as if it was supposed to be part of the ViewModel. You are making decisions based on ViewModel's internal state, something that only ViewModel should be allowed to do.

_manager.Upsert(studentObject);

Is that "UpdateOrInsert"? That is some weird naming convention.

Another thing that confuses me is that you seem to be using MVC-like web call implementation, but you are using ViewModels. How does that even work? I always tied ViewModels with UI, not with web.