用提取方法重构，是否总是一个好主意？

Jeff 发表于 Dev

杰夫

因此，我花了大约10到20分钟来重构〜30行方法。结果：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的提取方法，我没有手动执行此操作。

丹尼尔·希尔加斯

我认为通过重构，您会使情况变得更糟，甚至更糟。

我的第一个想法如下所示：

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