Refactoring with Extract Method, is it always a good idea?

Question

So I spent like 10-20 minutes refactoring a ~30 line method. The result: 74 lines. That's not good in my opinion. Sure, it MAY be a bit more readable, but you still have to jump to each method to figure out the details. Also, extracting all those methods gave me a hard time figuring out good names for them. And what if in the future when I am refactoring a method and want to use an existing method name with a completely different signature? It get's hard to read - atleast that's what I think.

Here is my code before refactoring:

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

Now, this is the refactored version:

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

Honestly, I prefer the first version. Am I missing something here? When refactoring methods with a few control-flow statements, it gets ugly.

Should I stick with the non-refactored version? Could this be done better?

EDIT: Based on the comments, I want to point out that I used ReSharper's Extract Method, I didn't do this manually.

Answer 1

I think with your refactoring, you made things worse, way worse.

My first take on it would look something like this:

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

This approach extracts methods that encapsulate a specific concept / functionality that very well might be needed by other methods.