Highlight Current Link

Question

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?

Answer 1

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