在不影响业务逻辑的情况下降低圈复杂度

Question

考虑这个方法：

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
    }
}

此方法有很多退出点，圈复杂度为 8。我们的最佳实践表明它不应大于 5.

是不是因为有多个IF？
我该怎么做才能重构它以减少退出点？

提前致谢。

Answer 1

这是我对上述问题的评论摘要

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

“5”听起来有点低。 nDepend 和微软分别推荐“30" and "25”

n依赖：

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)

微软：

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" -

是的 ||

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

一个简单的方法是只采用方法和 "split into smaller methods"。即，不是将所有 if 都放在一个方法中，而是将您的一些 if 逻辑移动到一个或多个方法中，每个方法调用另一个。

例如

class Class1
{
    class  Hobbit
    {
         
    }

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

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

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

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

        }
    }
}

可以通过将最里面的 if 组移动到单独的方法中来改进：

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

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

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

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

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

但是，在“5”，我认为您的代码不需要为了减少 CC 的目的而进行重构。

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

根据nDepend，退出点如return不算在内：

Following expressions are not counted for CC computation:

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

Answer 2

查看您的代码，很明显您的高圈复杂度和难以重构的方式是糟糕设计的指标（例如代码味道）。让我们稍微回顾一下。

_helper
_manager

这些是什么东西？为什么他们有这样模棱两可的名字？如果您找不到其他合适的名称，则说明您的关注点分离是错误的。

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

我什至无法想象这些方法是如何工作的。一些通用助手如何知道如何从通用 ViewModel 获取 "student"？这两种方法有什么区别？

    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();
    }

这看起来确实应该是 ViewModel 的一部分。您正在根据 ViewModel 的内部状态做出决定，只有 ViewModel 才应该被允许这样做。

_manager.Upsert(studentObject);

是"UpdateOrInsert"吗？这是一些奇怪的命名约定。

另一件让我感到困惑的事情是，您似乎在使用类似 MVC 的网络调用实现，但您使用的是 ViewModel。那是如何工作的？我总是将 ViewModels 与 UI 绑定，而不是与 web.

绑定

在不影响业务逻辑的情况下降低圈复杂度

Decreasing the Cyclomatic Complexity without affecting the business logic

c#

cyclomatic-complexity