1
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – George Mauer Feb 29 '12 at 14:38
  • \$\begingroup\$ 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. \$\endgroup\$ – Chase Florell Feb 29 '12 at 14:41
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.