Highlight Current Link

I’m using this technique for highlighting current links (e.g. how the “Questions” link is highlighted on this very page you’re looking at). I changed the code a little bit and came up with this extension method:

    public static MvcHtmlString MenuLink(
        this HtmlHelper helper,
        string content, string action, string controller)
    {
        var routeData = helper.ViewContext.RouteData.Values;
        var currentController = routeData["controller"];
        var currentAction = routeData["action"];

        var builder = new TagBuilder("li")
        {
            InnerHtml = content
        };
        if (String.Equals(action, currentAction as string,
                  StringComparison.OrdinalIgnoreCase)
            &&
           String.Equals(controller, currentController as string,
                   StringComparison.OrdinalIgnoreCase))
        {
            builder.AddCssClass("active");
            return MvcHtmlString.Create(builder.ToString());
        }
        return MvcHtmlString.Create(builder.ToString());
    }

It works fine, But that’s some ugly code. I use the extension method this way:

@Html.MenuLink("<a href=" + @Url.Action("MainPage", "Ticket") + "><i class='fa  fa-fw fa-home'></i>Ticket Page</a>", "MainPage", "Ticket")

As you can see, part of that isn’t strongly typed.

Do you have any ideas to improve the extension method?

One thing that I see that I would change is the if statement

    if (String.Equals(action, currentAction as string,
              StringComparison.OrdinalIgnoreCase)
        &&
       String.Equals(controller, currentController as string,
               StringComparison.OrdinalIgnoreCase))
    {
        builder.AddCssClass("active");
        return MvcHtmlString.Create(builder.ToString());
    }

I personally don’t like having a conditional that spans over multiple lines, so the first thing that I would do is to take the boolean conditions and make them into boolean variables and then use them inside the conditional. I think it would be easier to read.

bool isCurrentAction = String.Equals(action, currentAction as string,
          StringComparison.OrdinalIgnoreCase);
bool isCurrentController = String.Equals(controller, currentController as string,
           StringComparison.OrdinalIgnoreCase);
if (isCurrentAction && isCurrentController)
{
    builder.AddCssClass("active");
    return MvcHtmlString.Create(builder.ToString());
}

And since you are returning builder immediately after the if statement, there isn’t really a reason to return inside the if statement, it is rather redundant. Just delete that from the if block

bool isCurrentAction = String.Equals(action, currentAction as string,
          StringComparison.OrdinalIgnoreCase);
bool isCurrentController = String.Equals(controller, currentController as string,
           StringComparison.OrdinalIgnoreCase);
if (isCurrentAction && isCurrentController)
{
    builder.AddCssClass("active");
}