用Extract Method重构,总是一个好主意吗？

Question

所以我花了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的提取方法,我没有手动这样做.

Answer 1

我认为通过你的重构,你让事情变得更糟,更糟糕.

我的第一个看法是这样的:

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

此方法提取封装特定概念/功能的方法,这些概念/功能很可能是其他方法所需要的.