3

所以我花了大约 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 的提取方法，我没有手动执行此操作。

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 回答