Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I made a function that returns the number displayed for pagination. I want to get something like this (parentheses is the only show where the active site):

if pages < 10

1 2 3 4 5 6 7 8 9 10

if pages > 10

1 2 3 ... 20 [30] 40 ... 78 79 80

where [30] is active page. If active page < 3

1 2 [3] 4 ... 20 30 40 ... 78 79 80

etc. My code:

    count_all - number of all items
    items_on_page - items displayed on one page
    page - current page

    countpages = int(float(count_all+(items_on_page-1))/float(items_on_page))
    linkitems = 10

    if countpages > (linkitems-1):
        countpagesstr = list()
        for i in range(1,int(float(linkitems+(2))/float(3))):
            countpagesstr += [str(i)]

        countpagesstr += ['...']
        sr = int(countpages/2)
        for i in range(sr-1, sr+1):
            countpagesstr += [str(i)]

        countpagesstr += ['...']
        for i in range(countpages-3, countpages):
            countpagesstr += [str(i)]
    else:
        cp = list()
        for c in range(1,countpages+1):
            cp.append(str(c))
        countpagesstr = cp

    return countpagesstr

How to do it better?

share|improve this question

1 Answer

up vote 3 down vote accepted
  1. Your code doesn't run as written. It needs a function definition.

  2. Your design needs work. Suppose the current page is 30 and the list of pages says "... 20 [30] 40 ..." as in your example. How do I get to page 29 or page 31? It doesn't look as if these page numbers will ever appear in the list.

    The more usual design is to always show a small number of pages at the beginning and end, and a small number of pages adjacent to the current page. So that the list of pages would look more like "1 2 3 ... 28 29 [30] 31 32 ... 78 79 80".

  3. The expression

    int(float(count_all+(items_on_page-1))/float(items_on_page))
    

    is unnecessarily complex. You can use Python's floor division operator (//) to achieve the same thing without converting to floats and back again:

    (count_all + items_on_page - 1) // items_on_page
    
  4. Your variables are poorly and unsystematically named. count_all, for example. All what? Or countpagesstr, which sounds like it might be a string, but is in fact a list. You use underscores in some cases (count_all) but not in others (countpages).

  5. It is easiest conceptually to separate the two steps of deciding which page numbers to show and showing them in order with ellipses.

    In the code below I build the set of the page numbers from the first three pages, the five pages around the current page, and the last three pages. I use Python's sets so that I can take this union without having to worry about duplicate page numbers.

    def abbreviated_pages(n, page):
        """
        Return a string containing the list of numbers from 1 to `n`, with
        `page` indicated, and abbreviated with ellipses if too long.
    
        >>> abbreviated_pages(5, 3)
        '1 2 [3] 4 5'
        >>> abbreviated_pages(10, 10)
        '1 2 3 4 5 6 7 8 9 [10]'
        >>> abbreviated_pages(20, 1)
        '[1] 2 3 ... 18 19 20'
        >>> abbreviated_pages(80, 30)
        '1 2 3 ... 28 29 [30] 31 32 ... 78 79 80'
        """
        assert(0 < n)
        assert(0 < page <= n)
    
        # Build set of pages to display
        if n <= 10:
            pages = set(range(1, n + 1))
        else:
            pages = (set(range(1, 4))
                     | set(range(max(1, page - 2), min(page + 3, n + 1)))
                     | set(range(n - 2, n + 1)))
    
        # Display pages in order with ellipses
        def display():
            last_page = 0
            for p in sorted(pages):
                if p != last_page + 1: yield '...'
                yield ('[{0}]' if p == page else '{0}').format(p)
                last_page = p
    
        return ' '.join(display())
    
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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