如何重构这段 C# 代码，使其更容易阅读？

Question

这是我的一个控制器中的操作 - 我在整个项目的控制器中有类似的大型方法。

我正在尝试学习在哪里放置这些东西以及如何清理代码。我刚开始做这个，如果我看到一个好的例子来改变我的一些代码，它很可能会教我如何处理大量的代码。

这是我的操作：

public ActionResult Index(string sortOrder, string currentFilter, string searchString, int? page)
{
    ViewBag.CurrentSort = sortOrder;
    ViewBag.TitleSortParm = String.IsNullOrEmpty(sortOrder) ? "Title desc" : "";
    ViewBag.CreditsSortParm = sortOrder == "Credits" ? "Credits desc" : "Credits";
    ViewBag.ElectiveSortParm = sortOrder == "Elective" ? "Elective desc" : "Elective";
    ViewBag.InstructorSortParm = sortOrder == "Instructor" ? "Instructor desc" : "Instructor";
    ViewBag.YearSortParm = sortOrder == "Year" ? "Year desc" : "Year";
    ViewBag.AttendingDaysSortParm = sortOrder == "AttendingDays" ? "AttendingDays desc" : "AttendingDays";
    ViewBag.AttendanceCapSortParm = sortOrder == "AttendanceCap" ? "AttendanceCap desc" : "AttendanceCap";
    ViewBag.StartDateSortParm = sortOrder == "StartDate" ? "StartDate desc" : "StartDate";
    ViewBag.LocationSortParm = sortOrder == "Location" ? "Location desc" : "Location";
    ViewBag.ParishSortParm = sortOrder == "Parish" ? "Parish desc" : "Parish";
    ViewBag.DescriptionSortParm = sortOrder == "Description" ? "Description desc" : "Description";
    ViewBag.ApprovedSortPArm = sortOrder == "Approved" ? "Approved desc" : "Approved";
    ViewBag.CompletedSortPArm = sortOrder == "Completed" ? "Completed desc" : "Completed";
    ViewBag.ArchivedSortPArm = sortOrder == "Archived" ? "Archived desc" : "Archived";

    if (Request.HttpMethod == "GET")
    {
        searchString = currentFilter;
    }
    else
    {
        page = 1;
    }
    ViewBag.CurrentFilter = searchString;

    var courses = from s in db.Courses
                    select s;
    if (!String.IsNullOrEmpty(searchString))
    {
        courses = courses.Where(s => s.Title.ToUpper().Contains(searchString.ToUpper()));
    }
    switch (sortOrder)
    {
        case "Title desc":
            courses = courses.OrderByDescending(s => s.Title);
            break;
        case "Credits":
            courses = courses.OrderBy(s => s.Credits);
            break;
        case "Credits desc":
            courses = courses.OrderByDescending(s => s.Credits);
            break;
        case "Elective":
            courses = courses.OrderBy(s => s.Credits);
            break;
        case "Elective desc":
            courses = courses.OrderByDescending(s => s.Credits);
            break;
        case "Instructor":
            courses = courses.OrderBy(s => s.Instructor.LastName);
            break;
        case "Instructor desc":
            courses = courses.OrderByDescending(s => s.Instructor.LastName);
            break;
        case "Year":
            courses = courses.OrderBy(s => s.Year);
            break;
        case "Year desc":
            courses = courses.OrderByDescending(s => s.Year);
            break;
        case "AttendingDays":
            courses = courses.OrderBy(s => s.AttendingDays);
            break;
        case "AttendingDays desc":
            courses = courses.OrderByDescending(s => s.AttendingDays);
            break;
        case "AttendanceCap":
            courses = courses.OrderBy(s => s.AttendanceCap);
            break;
        case "AttendanceCap desc":
            courses = courses.OrderByDescending(s => s.AttendanceCap);
            break;
        case "StartDate":
            courses = courses.OrderBy(s => s.StartDate);
            break;
        case "StartDate desc":
            courses = courses.OrderByDescending(s => s.StartDate);
            break;
        case "Location":
            courses = courses.OrderBy(s => s.Location);
            break;
        case "Location desc":
            courses = courses.OrderByDescending(s => s.Location);
            break;
        case "Parish":
            courses = courses.OrderBy(s => s.Parish);
            break;
        case "Parish desc":
            courses = courses.OrderByDescending(s => s.Parish);
            break;
        case "Description":
            courses = courses.OrderBy(s => s.Description);
            break;
        case "Description desc":
            courses = courses.OrderByDescending(s => s.Description);
            break;
        case "Approved":
            courses = courses.OrderBy(s => s.Approved);
            break;
        case "Approved desc":
            courses = courses.OrderByDescending(s => s.Approved);
            break;
        case "Completed":
            courses = courses.OrderBy(s => s.Completed);
            break;
        case "Completed desc":
            courses = courses.OrderByDescending(s => s.Completed);
            break;
        case "Archived":
            courses = courses.OrderBy(s => s.Archived);
            break;
        case "Archived desc":
            courses = courses.OrderByDescending(s => s.Archived);
            break;
        default:
            courses = courses.OrderBy(s => s.Title);
            break;
    }
    int pageSize = 4;
    int pageNumber = (page ?? 1);
    return View(courses.ToPagedList(pageNumber, pageSize));
}

我该如何使上述代码更易读？我应该将其中的部分提取为单独的方法并将它们移至控制器底部吗？还是将这些方法放在另一个文件中，并在此引用它们？ 请记住，我正在学习，希望可以更加清晰易懂。

- Ecnalyr

---

不，把被调用的方法直接放在调用它的方法下面是合适的，这样两年后维护代码的开发人员就不必到处搜索。代码应该像一本书一样阅读，一个事情接着一个事情，依次往下进行。 - CD Smith

希望我能为“直接在调用者下面调用方法”给@AlfalfaStrange加上+10。 - Phillip Schmidt

@PhillipSchmidt 听起来你成了“隐藏函数”的受害者。 - CD Smith

@AlfalfaStrange，你比你知道的更重要。到了一定程度，当我在一个逻辑位置看到一个方法时，它会让我想请所有人吃午餐或其他什么东西。 - Phillip Schmidt

当你的键盘上有那个可爱的F12按钮时，你怎么可能找不到方法呢？ - James Hull

Answer 1

你如何改变代码纯属主观，这是一个需要感受和努力去成为更好的开发人员的创造性过程。

但作为伪代码示例，目标是使您的方法更小。不，真的，更小！

使函数名称非常有意义，并确保它们告诉程序员该方法的真正意图。这只是一种结构示例：

public class Something {

   public ActionResult Index() {
       MakeCallToFunction();
       var something = GetSomething();
       var somethingElse = GetSomethingElse(something);
       var model = new SomeViewModel(somethingElse);
       return View(model);
   }

   private void MakeCallToFunction () {
       ...
   }

   private string GetSomething() {
       ...
       return someVal;
   }

   private SomeObject GetSomethingElse(string something) {
       ...
       return someBigObject;
   }
}

或者这意味着您将所有这些小函数组合成一个类。

public class Something {

   public ActionResult Index(int id) {
       var model = new SomeRelatedStuff.Load(id);
       return View(model);
   }
}

private class SomeRelatedStuff {

   public SomeRelatedStuff()

   public static SomeRelatedStuff Load(int id) {
       var something = GetSomething();
       var somethingElse = GetSomethingElse(something);
       var model = new SomeViewModel(somethingElse);
       MakeObject();
       return this;
   }

   private string GetSomething() {
       ...
       return someVal;
   }

   private SomeObject GetSomethingElse(string something) {
       ...
       return someBigObject;
   }

   private void MakeObject () {
       ...
   }

}

顺便提一下，请务必去阅读罗伯特·马丁（叔叔鲍勃）的《代码整洁之道》，这个周末至少读完前四章。然后再读一遍。

- CD Smith

---

我讨厌你现在告诉我这个，而不是昨天，那时我有足够的时间在离开城镇度假四天之前让亚马逊把它隔夜送到我手上。看来我得在某个建筑物里找找了。感谢你的提示！ - Ecnalyr

Answer 2

首先，摆脱你的魔法字符串。把排序顺序定义为一个枚举类型，就像这样：

public enum ActionSortOrder {
    Credits,
    Elective,
    ...
}

ActionSortOrder order = ActionSortOrder.Credits;

这比使用未定义的字符串要容易管理得多，如果枚举成员的功能需要一些解释，您可以记录枚举成员的文档。

其次，方法顶部的行有点令人困惑：

 ViewBag.CreditsSortParm = sortOrder == "Credits" ? "Credits desc" : "Credits";

如果您将“Credits”作为排序顺序传递，它会默认为降序？如果没有，它会默认为升序？为什么？为什么不提供升序/降序排序选项（再次使用枚举而不是魔法字符串）？为什么每次调用此方法时都保存所有这些排序顺序？这些设置变量在哪里使用？您只在此方法中使用传递的顺序。

另外，不要担心开关语句看起来很大。开关语句通常只用于避免一堆if-else语句。因此，在实践中，大多数开关有相当多的情况。

了解更多关于您代码的信息将有助于评估它。

- rob_williams

---

我正在展示一张表格，其中显示了所有可以注册的“学校课程”的详细信息。每个排序参数都是可以进行排序的特征之一。因此，他们可以按照学分最多或最少的课程进行排序，或按照标题的字母顺序进行排序等。 - Ecnalyr