3

これは私のコントローラー内のアクションです。プロジェクト全体でコントローラーに同様の大きなメソッドがあります。

私はこれらのものをどこに置き、どのように掃除するかを学ぼうとしています. 私はこれを行うのが初めてです。自分のコードの一部を変更する方法の良い例を見れば、大量のコードにそれを行う方法を教えてくれるでしょう。

これが私の行動です：

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

上記のコードを読みやすくするにはどうすればよいですか? その一部を個別のメソッドとして移動し、コントローラーの下部に移動するだけですか? メソッドをどこか別のファイルに入れて、ここで参照しますか?

私は学んでおり、明晰さを楽しんでいることを忘れないでください。

4

2 に答える 2

2

コードをどのように変更するかは純粋に主観的なものであり、より優れた開発者になるためには、感じて戦い抜く必要がある創造的なプロセスです。

しかし、疑似コードの例として、目的はメソッドを小さくすることです。いいえ、本当に、さらに小さいです！

関数名を非常に意味のあるものにし、メソッドの真の意図が何であるかをプログラマーに伝えるようにしてください。これは構造の単なるサンプルです:

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 () {
       ...
   }

}

余談ですが、Robert Martin (Uncle Bob) による Clean Code を入手して、今週末に少なくとも最初の 4 章を読んでください。それからそれらをもう一度読んでください。

于 2012-05-31T14:07:44.647 に答える
2

まず、魔法の糸を取り除きます。次のように、並べ替え順序を列挙型にします。

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

ActionSortOrder order = ActionSortOrder.Credits;

これは、未定義の文字列を使用するよりもはるかに扱いやすく、その機能について説明が必要な場合は、列挙メンバーを文書化できます。

第二に、メソッドの上部にある行はやや混乱しています。

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

ソート順として「Credits」を渡すと、デフォルトで降順になりますか? そうしないと、デフォルトで昇順になりますか？なんで？昇順/降順のオプションを提供しないのはなぜですか (ここでもマジック文字列ではなく列挙型を使用)。そして、このメソッドが呼び出されるたびに、これらすべての並べ替え順序が保存されるのはなぜですか? これらの設定変数はどこで使用されますか? このメソッド内で渡された順序のみを使用します。

また、スイッチが大きく見えることを心配する必要はありません。通常、switch ステートメントは、一連の if-else ステートメントを避けるためにのみ使用されます。そのため、ほとんどのスイッチには、実際にはかなりの数のケースがあります。

コードについて詳しく知ることは、コードの評価に役立ちます。

于 2012-05-31T14:54:09.133 に答える