MVC pager helper

Question

I've got this MVC pager helper and it's working exactly as expected. Is there a prettier or more performant way to build this out?

Also, I know that the StringBuilder might not be the best way. Would a TagBuilder be a better approach?

<Extension()>
Public Function Pager(helper As HtmlHelper,
 urlPrefix As String,
 totalRecords As Integer,
 currentPage As Integer) As MvcHtmlString
 ' Get out if we have 5 or less records
 If totalRecords <= 5 Then Return Nothing
 ' Make sure we're not getting invalid pages
 If currentPage <= 0 Then currentPage = 1
 ' Setup our initial variables
 Dim sb1 As New StringBuilder(),
 totalPages = (Math.Round((totalRecords / 5) + 0.5)),
 startingPoint,
 linksAfterCurrent,
 endPoint,
 i
 ' Set boundries for inner link numbers
 Select Case currentPage
 Case totalPages : startingPoint = currentPage - 5
 Case (totalPages - 1) : startingPoint = currentPage - 4
 Case Else : startingPoint = currentPage - 3
 End Select
 Select Case currentPage
 Case 1 : linksAfterCurrent = currentPage + 4
 Case 2 : linksAfterCurrent = currentPage + 3
 Case Else : linksAfterCurrent = currentPage + 2
 End Select
 sb1.Append("<div id=""pagercontainer""><ul class=""pager"">")
 ' Display the previous button and first button
 If currentPage > 1 AndAlso startingPoint >= 1 Then
 sb1.AppendLine([String].Format("<li><a href=""{0}1"" title=""go to page 1"">1</a></li>", urlPrefix))
 sb1.AppendLine([String].Format("<li><a href=""{0}{1}"" title=""go to page {1}"">&laquo;</a></li>", urlPrefix, startingPoint))
 sb1.AppendLine("&nbsp;&nbsp;&nbsp;")
 End If
 ' Generate the inner numbers
 i = startingPoint
 While (i < linksAfterCurrent)
 If (i >= 0) AndAlso
 (i < totalPages) Then
 sb1.AppendLine([String].Format("<li><a href=""{0}{1}"" {2} title=""go to page {1}"">{1}</a></li>",
 urlPrefix,
 i + 1,
 If(i + 1 = currentPage, "class=""youarehere""", String.Empty)))
 End If
 i += 1
 End While
 endPoint = i
 ' Display the next button and the last button
 If (currentPage < endPoint) AndAlso
 (endPoint < totalPages) Then
 sb1.AppendLine("&nbsp;&nbsp;&nbsp;")
 sb1.AppendLine([String].Format("<li><a href=""{0}{1}"" title=""go to page {1}"">&raquo;</a></li>", urlPrefix, endPoint + 1))
 sb1.AppendLine([String].Format("<li><a href=""{0}{1}"" title=""go to page {1}"">{1}</a></li>", urlPrefix, totalPages.ToString()))
 End If
 sb1.Append("</ul></div><div class=""clear""></div>")
 Return MvcHtmlString.Create(sb1.ToString())
End Function
 sb1.Append("</ul></div><div class=""clear""></div>")
 Return MvcHtmlString.Create(sb1.ToString())
End Function

• George Mauer: I would not worry about performance. There's no way this code runs often enough to be a problem. The general rule-of-thumb is don't worry about performance until you start having problems.
• Chase Florell: Yeah, I get that, and the code works as is. It's just ugly IMO. I guess I'm more concerned about the structure than I am about the performance at this point.

Answer 1

How about something like this?

// Put this in its own class so that you can have private methods without being confused with your other HtmlHelpers
public class PagerGenerator {
 //extension method hook so this gets added to HtmlHelper
 public static MvcHtmlString Pager(this HtmlHelper helper, string urlPrefix,int totalRecords,int currentPage) {
 return new PagerGenerator(helper, urlPrefix).Generate(totalRecords,currentPage);
 }
 public MvcHtmlString Generate(this HtmlHelper helper, string urlPrefix,int totalRecords,int currentPage)
 if(totalRecords <= recordsPerPage)
 return null;
 if(currentPage <= 0) currentPage = 1;
 var totalPages = Math.Floor(totalRecords / recordsPerPage); //Math.Floor is the function you're looking for
 var startingPoint = CalcualateStartingPoint(currentPage, totalPages);
 var linksAfterCurrent = CalculateNumberOfLinksAfterCurrent(currentPage, totalPages);
 //I have no problem with using StringBuilder here actually, tagbuilder is fine too if you want, either should be fine.
 var sb = new StringBuilder(); 
 sb.Append("<div id=""pagercontainer""><ul class=""pager"">")
 AppendStartingLinks(sb, currentPage, startingPoint);
 // Personally I hate for loops. foreach is better. An alternative is to generate all i's, 
 // Linq-Select the strings you want, and to append them all in one shot, but this is probably a bit
 // easier to read
 foreach(var i in Enumerable.Range(startingPoint, linksAfterCurrent-1))
 AppendPageLink(sb, i, i.ToString(), i == currentPage-1? "class=""youarehere""" : String.Empty);
 AppendTerminalLinks(sb, linksAfterCurrent, totalPages);
 sb.Append("</ul></div><div class=""clear""></div>");
 return MvcHtmlString.Create(sb1.ToString())
 }
 // move this to a constant so it's not just a magic number floating around
 const int recordsPerPage = 5;
 private readonly HtmlHelper htmlHelper;
 private readonly string urlPrefix; //generally a good idea to move to the constructor params that you're passing around a bunch
 public PagerGenerator(HtmlHelper helper, string urlPrefix) {
 this.htmlHelper = helper;
 this.urlPrefix = urlPrefix;
 }
 private static int CalcualateStartingPoint(currentPage, totalPages) {
 // Nothing wrong with case statements here, I just prefer ternary notation
 var startingPoint = 
 totalPages == currentPage ? currentPage - 5 :
 (totalPages -1) ==currentPage ? currentPage - 4 :
 currentPage - 3;
 return startingPoint > 0 ? startingPoint : 0;
 }
 private static int CalculateNumberOfLinksAfterCurrent(currentPage, totalPages) {
 var l =
 1 == currentPage ? currentPage + 4 :
 2 == currentPage ? currentPage + 3 :
 currentPage + 2 
 return l > totalPages ? totalPages : l;
 }
 private void AppendPageLink(StringBuilder sb, int page) {
 AppendPageLink(sb, page, page.toString());
 }
 private void AppendPageLink(StringBuilder sb, int page, string text, string additionalAttributes = null) {
 sb.AppendLine(String.Format("<li><a href=""{0}{1}"" title=""go to page {1}"" {3} >{2}</a></li>", 
 this.urlPrefix, page, text, additionalAttributes == null ? String.Empty : additionalAttributes));
 }
 private void AppendStartingLinks(StringBuilder sb, int currentPage, int startingPoint) {
 if(currentPage > 1 && startingPoint >= 1) {
 AppendPageLink(sb, 1);
 AppendPageLink(sb, startingPoint, "&laquo;");
 sb.AppendLine("&nbsp;&nbsp;&nbsp;");
 }
 }
 private void AppendTerminalLinks(StringBuilder sb, int linksAfterCurrent, int totalPages) {
 If (currentPage < linksAfterCurrent && linksAfterCurrent < totalPages) {
 sb.AppendLine("&nbsp;&nbsp;&nbsp;");
 AppendPageLink(sb, linksAfterCurrent+1)
 AppendPageLink(sb, totalPages)
 }
 }
}

I rewrote it in C# because that is easier for me (and also Sublime Text doesn't have a VB mode) so you'll have to convert the syntax back to VB.

The overall structure of this is not bad. I moved the extension logic to its own class. This is so that you can have private helper methods which I created a bunch of. I am a big fan of the extract-private-method refactoring. If you read through the public Generate method now you will see it's very easy to pick out what's going on, whereas before you had to pick through the syntax.

Private methods are your friends.

Please Note: I have neither checked the logic nor compiled it. There were a couple points where it seemed like your logic could be simplified so I did (the calculation for endPoint seems like it would always be linksAfterCurrent for example). You should double check everything but the refactorings of extract-class and extract-private-method is the key to cleaning this up.