current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 10 down vote favorite

I have written a small function to generate an array of page numbers for displaying a pagination. The inputs to the function is an active page number p, and the max page number. The function will return an array of 5 possible page numbers available near the active page.

Example input, output:

pages(1,10): [1,2,3,4,5]

pages(10,10): [6,7,8,9,10]

pages(5,10): [3,4,5,6,7]

The function is:

def pages(p,max):
    a=[p]
    while len(a)<5 and not (a[0]==1 and a[-1]==max):
        if a[0]>1:
            a.insert(0,a[0]-1)
        if a[-1]<max:
            a.append(a[-1]+1)
    return a

Is there anything to do to improve this code, or is there any other simpler methods to achieve this?

share|improve this question
up vote 7 down vote accepted

1. Review

  1. The function needs a docstring to explain what it does, with maybe some examples. The explanation in your post is very good: this is exactly the kind of thing that should go into the docstring.

  2. The number 5 is arbitrary, so I would make it a parameter to the function.

  3. The implementation takes time proportional to the number of entries in the page range. It would be more elegant to compute the endpoints of the range directly, taking constant time.

2. Revised code

def page_range(page, last, span=5):
    """Return a range of page numbers around page, containing span pages
    (if possible). Page numbers run from 1 to last.

    >>> list(page_range(2, 10))
    [1, 2, 3, 4, 5]
    >>> list(page_range(4, 10))
    [2, 3, 4, 5, 6]
    >>> list(page_range(9, 10))
    [6, 7, 8, 9, 10]

    """
    return range(max(min(page - (span - 1) // 2, last - span + 1), 1),
                 min(max(page + span // 2, span), last) + 1)
share|improve this answer
    
Just remembered I shouldn't use max as a variable name. – rahul May 30 '15 at 9:52

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.