3

そのため、約 30 行のメソッドのリファクタリングに 10 ～ 20 分ほど費やしました。結果: 74 行。私の意見では、それは良くありません。確かに、もう少し読みやすいかもしれませんが、詳細を把握するには、各メソッドにジャンプする必要があります。また、これらのメソッドをすべて抽出すると、適切な名前を見つけるのに苦労しました。また、将来、メソッドをリファクタリングしていて、まったく異なるシグネチャを持つ既存のメソッド名を使用したい場合はどうすればよいでしょうか? 読むのが難しくなります - 少なくとも私はそう思います。

リファクタリング前の私のコードは次のとおりです。

    public ActionResult Confirm(string id)
    {
        if (string.IsNullOrEmpty(id))
        {
            if (! IsLoggedIn())
            {
                return RedirectToAction("Login");
            }

            if(User.User.Confirmed)
            {
                return RedirectToAction("Index");
            }
            return View("PendingConfirmation");
        }

        int parsedId;
        if (!int.TryParse(id, out parsedId))
        {
            return Http(400, View("BadRequest", model: "EC2007: Could not parse int"));
        }

        return Try(() =>
        {
            UserBusinessLogic.ConfirmUser(parsedId);
            _authentication.SetAuthCookie(parsedId.ToString(CultureInfo.InvariantCulture), true);
            return RedirectToAction("Index");
        }, (code, errorCode) => Http(code, GenericErrorView(null, null, errorCode)));
    }

さて、これはリファクタリングされたバージョンです:

    /// <summary>
    ///     Confirms the specified id.
    /// </summary>
    /// <param name="id">The id.</param>
    /// <returns></returns>
    public ActionResult Confirm(string id)
    {
        int parsedId;
        ActionResult actionResult;
        if (! AssertConfirmConditions(id, out parsedId, out actionResult))
        {
            return actionResult;
        }
        return Try(() => InternalConfirmUser(parsedId), (code, errorCode) => Http(code, GenericErrorView(null, null, errorCode)));
    }

    private ActionResult InternalConfirmUser(int parsedId)
    {
        UserBusinessLogic.ConfirmUser(parsedId);
        _authentication.SetAuthCookie(parsedId.ToString(CultureInfo.InvariantCulture), true);
        return RedirectToAction("Index");
    }

    private bool AssertConfirmConditions(string id, out int parsedId, out ActionResult actionResult)
    {
        actionResult = null;
        parsedId = 0;
        return 
            ! ShouldRedirectAwayFromConfirm(id, ref actionResult) 
            && CanParseId(id, ref parsedId, ref actionResult);
    }

    private bool CanParseId(string id, ref int parsedId, ref ActionResult actionResult)
    {
        if (int.TryParse(id, out parsedId))
        {
            return true;
        }
        actionResult = Http(400, View("BadRequest", model: "EC2007: Could not parse int"));
        return false;
    }

    private bool ShouldRedirectAwayFromConfirm(string id, ref ActionResult actionResult)
    {
        if (string.IsNullOrEmpty(id))
        {
            if (ShouldRedirectToLoginView(out actionResult)) return true;
            if (ShouldRedirectToIndex(ref actionResult)) return true;
            actionResult = View("PendingConfirmation");
            return true;
        }
        return false;
    }

    private bool ShouldRedirectToIndex(ref ActionResult actionResult)
    {
        if (User.User.Confirmed)
        {
            actionResult = RedirectToAction("Index");
            return true;
        }
        return false;
    }

    private bool ShouldRedirectToLoginView(out ActionResult actionResult)
    {
        actionResult = null;
        if (! IsLoggedIn())
        {
            actionResult = RedirectToAction("Login");
            return true;
        }
        return false;
    }

正直なところ、私は最初のバージョンの方が好きです。ここで何か不足していますか？少数の制御フロー ステートメントでメソッドをリファクタリングすると、見苦しくなります。

リファクタリングされていないバージョンに固執する必要がありますか? これはもっとうまくできるでしょうか？

編集:コメントに基づいて、ReSharper の Extract Method を使用したことを指摘したいと思います。これは手動では行いませんでした。

4

2 に答える 2

5

あなたのリファクタリングで、あなたは事態をさらに悪化させたと思います。

私の最初のテイクは次のようになります。

public ActionResult Confirm(string id)
{
    if (string.IsNullOrEmpty(id))
    {
        return HandleMissingId();
    }

    int parsedId;
    if (!int.TryParse(id, out parsedId))
    {
        return Http(400, View("BadRequest", model: "EC2007: Could not parse int"));
    }

    return Try(() =>
    {
        ConfirmUser(parseId);
        return RedirectToAction("Index");
    }, ShowGenericError);
}

private void ConfirmUser(int userId)
{
    UserBusinessLogic.ConfirmUser(userId);
    _authentication.SetAuthCookie(userId.ToString(CultureInfo.InvariantCulture), true);
}

private ShowGenericError(int code, int errorCode)
{
    return Http(code, GenericErrorView(null, null, errorCode));
}

private ActionResult HandleMissingId()
{
    if (! IsLoggedIn())
    {
        return RedirectToAction("Login");
    }

    if(User.User.Confirmed)
    {
        return RedirectToAction("Index");
    }
    return View("PendingConfirmation");
}

このアプローチは、他のメソッドで必要とされる可能性が非常に高い特定の概念/機能をカプセル化するメソッドを抽出します。

于 2013-09-10T14:16:15.920 に答える
1

私は一般的に、メソッドはコードを小さな断片に分割するためではなく、機能の再利用を容易にし、保守しやすくするために作成する必要があるという意見です。30 行のメソッドに含まれるコードがメソッド内またはプロジェクトの他の場所で再利用されていない場合、メソッドに問題はないと思います。何かを独自のメソッドに分割するかどうかを検討するときは、それがいつでも再利用できるものになるかどうかを確認してください。プログラムを通じて再利用される可能性が低いロジックであれば、それをリファクタリングする必要はありません。独自のメソッド（読み取りやデバッグが面倒になるほど長くなる場合を除きます）

于 2013-09-10T14:15:51.127 に答える