3
\$\begingroup\$

I didn't want my title to be too long so to clear up any confusion I'll explain better here what my code does.

It asks for the total pages of a book(PDF file in my case), and returns the page numbers in a specific order. The groups of 32 pages will each make a booklet that when all booklets are stapled together and laid one on top of the other will be a book ready for binding.

One issue with how I wrote the program is if "pages" is not a multiple of 4, then there will be extraneous pages at the end of the list (i couldn't think of a way to pop those). Another tid-bit, print_order doesn't need to be a list, it just needs to be page numbers separated by commas and I knew list items were separated by commas so having a list of ints would give me what I needed. The formula for printing a book on a single sided printer goes like this (n, f, f+1, n-1) where n is the last page, and f is the first page of each booklet/the whole book. The reason a long book needs to be broken into booklets is because if I take a stack of 100 papers and just fold the whole stack in half the spine would be too wide and the middle pages would be poking out of the cover. So using n and f as the first and last page, respectively, of smaller booklets allows me to print a whole book into something more practical.

I'm not worried about PEP formatting on this one I'm really just curious how this could be written more efficiently. I know python is extremely powerful and I'm not taking full advantage of what it has to offer. This is how I managed to organize my thoughts into doing what I required. I know that there are many out there who could do this in a much simpler way, and I just want to look those ways over so that I can improve my coding style!

pages = float(input("How many total pages is the book?" ))
num_booklets = pages/32
if (pages < 32):
    num_booklets = 1
pages_per_booklet = int(pages/num_booklets)
last_booklet = int(pages%32)


f = 1               #first page of the book
n = int(pages_per_booklet)  #last page of the first booklet
k = 1               #number of booklets completed
print_order = []


while (len(print_order) < pages):
    while ((len(print_order) < pages_per_booklet*k) 
    and (len(print_order) < pages)):
        print_order.extend([n, f, f+1, n-1])
        n -= 2
        f += 2
    k += 1
    n = pages_per_booklet * k
    f = 1 + (pages_per_booklet*(k-1))
    if ((len(print_order)+pages_per_booklet) > pages):
        n = int(pages)
        f = int(pages) - last_booklet


print(print_order)
\$\endgroup\$
  • \$\begingroup\$ After some more though I was able to come up with for page in range(1, int(pages/2)): if page%2 == 0: continue print_order.extend([int(pages), page, page+1, int(pages)-1]) pages -= 2 for the main loop. I just need to figure out how to break a long book into booklets of 32 so I can run that loop. \$\endgroup\$ – qazwsx598 Mar 10 '18 at 18:09
  • 1
    \$\begingroup\$ Your question seems interesting. I'll try to review it tomorrow but a first step could be to write a function taking a number of page and returning a print_order. Then you'd be able to write tests and optimising your code will be easier. \$\endgroup\$ – SylvainD Apr 9 '18 at 22:16
  • \$\begingroup\$ Why are you not worried about PEP8? As soon as you give your code to others to read, you should be, since they expect your code to "look normal". \$\endgroup\$ – Roland Illig Apr 10 '18 at 5:33
  • \$\begingroup\$ @Roland Illig You're right, I didn't think about that.. lol. I was just trying to get it working at the time \$\endgroup\$ – qazwsx598 Apr 10 '18 at 23:31
3
\$\begingroup\$

Style

Your code is not too far from following PEP8 and you know PEP8: it would have been a nice touch for volunteer reviewers to provide a piece of code even closer to what they expect. You'll find various tools (online or not) to check more or less precisely the compliancy of your code to PEP8. For most points, once you get used to it, it doesn't take any longer to apply as as you write the code.

Float ?

The following piece of code

pages = float(input("How many total pages is the book?"))

seems to imply that pages could be a float. As far as I can tell, this value corresponds to a number of page and an int would be more appropriate. Also, to be sure, I've tried giving a 0.5 value and the code is stuck in a loop.

Code organisation

Your code is the perfect example of a piece of code that could have been written as a clear function: input and output are both well-defined. Once you're done, you can even write unit-tests for it. Then, the code calling your function could be behind an if __name__ == "__main__": guard.

You'd get something like:

def page_order(pages):
    num_booklets = pages/32
    if (pages < 32):
        num_booklets = 1
    pages_per_booklet = int(pages/num_booklets)
    last_booklet = int(pages%32)

    f = 1               #first page of the book
    n = int(pages_per_booklet)  #last page of the first booklet
    k = 1               #number of booklets completed
    print_order = []

    while (len(print_order) < pages):
        while ((len(print_order) < pages_per_booklet*k) 
        and (len(print_order) < pages)):
            print_order.extend([n, f, f+1, n-1])
            n -= 2
            f += 2
        k += 1
        n = pages_per_booklet * k
        f = 1 + (pages_per_booklet*(k-1))
        if ((len(print_order)+pages_per_booklet) > pages):
            n = int(pages)
            f = int(pages) - last_booklet
    return (print_order)

tests = {
    0: [],
    1: [1, 1, 2, 0],
    2: [2, 1, 2, 1],
    3: [3, 1, 2, 2],
    4: [4, 1, 2, 3],
    5: [5, 1, 2, 4, 3, 3, 4, 2],
    6: [6, 1, 2, 5, 4, 3, 4, 3],
    7: [7, 1, 2, 6, 5, 3, 4, 4],
    8: [8, 1, 2, 7, 6, 3, 4, 5],
    9: [9, 1, 2, 8, 7, 3, 4, 6, 5, 5, 6, 4],
    10: [10, 1, 2, 9, 8, 3, 4, 7, 6, 5, 6, 5],
    11: [11, 1, 2, 10, 9, 3, 4, 8, 7, 5, 6, 6],
    12: [12, 1, 2, 11, 10, 3, 4, 9, 8, 5, 6, 7],
    13: [13, 1, 2, 12, 11, 3, 4, 10, 9, 5, 6, 8, 7, 7, 8, 6],
    14: [14, 1, 2, 13, 12, 3, 4, 11, 10, 5, 6, 9, 8, 7, 8, 7],
    15: [15, 1, 2, 14, 13, 3, 4, 12, 11, 5, 6, 10, 9, 7, 8, 8],
    16: [16, 1, 2, 15, 14, 3, 4, 13, 12, 5, 6, 11, 10, 7, 8, 9],
    17: [17, 1, 2, 16, 15, 3, 4, 14, 13, 5, 6, 12, 11, 7, 8, 10, 9, 9, 10, 8],
    18: [18, 1, 2, 17, 16, 3, 4, 15, 14, 5, 6, 13, 12, 7, 8, 11, 10, 9, 10, 9],
    19: [19, 1, 2, 18, 17, 3, 4, 16, 15, 5, 6, 14, 13, 7, 8, 12, 11, 9, 10, 10],
}

if __name__ == "__main__":
    if False:  # Interactive
        pages = int(input("How many total pages is the book?" ))
        print(page_order(pages))
    else:  # Automatic tests
        for inpu, expected_val in tests.items():
            res = page_order(inpu)
            if res != expected_val:
                print(inpu, res, expected_val)
        print("DONE")

divmod

When you perform both a division and a modulo operation with the same values, you can use the more concise and faster divmod builtin function.

def page_order(pages):
    num_booklets, last_booklet = divmod(pages, 32)
    if pages < 32:
        num_booklets = 1

Then I do not understand enough what your code is trying to achieve to try to be able to help more. For the time being, the code looks like:

def page_order(pages):
    num_booklets, last_booklet = divmod(pages, 32)
    if pages < 32:
        num_booklets = 1
    pages_per_booklet = pages/num_booklets

    f = 1  #first page of the book
    n = pages_per_booklet  #last page of the first booklet
    k = 1  # number of booklets completed
    print_order = []

    while len(print_order) < pages:
        while len(print_order) < min(pages, pages_per_booklet*k):
            print_order.extend([n, f, f+1, n-1])
            n -= 2
            f += 2
        k += 1
        n = pages_per_booklet * k
        f = 1 + (pages_per_booklet*(k-1))
        if len(print_order) + pages_per_booklet > pages:
            n = pages
            f = pages - last_booklet
    return print_order
\$\endgroup\$
  • \$\begingroup\$ Firstly, thanks for your answer, I had never seen divmod before.. its really cool! Also, there were a lot of bugs with the code that you're reviewing, It had passed a few tests originally, but when I went to use it practically it was giving me the wrong order of numbers. I re-wrote the script down below. I went back and tried to use the (below) code a week or so ago and was getting an error, even though it had worked fine three weeks prior.. not sure why? I'll test it again tomorrow when I'm back on that computer \$\endgroup\$ – qazwsx598 Apr 11 '18 at 0:10
  • \$\begingroup\$ The simplest way I can describe what I was trying to do is: -Input an integer numberOfPages -Create lists, booklets, of equal length - the last list may be shorter -Arrange the ints in each list following this formula [n, f, f+1, n-1] -if the input is 4 the booklet should read [4, 1, 2, 3] -if the input is 8 the booklet should read [8, 1, 2, 7, 6, 3, 4, 5] and so on for increasing values of the input -lastly, print a single list of every booklet to create the book \$\endgroup\$ – qazwsx598 Apr 11 '18 at 0:16
  • \$\begingroup\$ I'm glad you like my answer. If you want to have another version of your code reviewed, please open a new question for it (stating that this is a rework of this question). Also, please make sure that your code is as good as it can be (documented ? follows PEP8? with test? taking into account comments from my answer?) so that you save everyone time including yours. \$\endgroup\$ – SylvainD Apr 11 '18 at 7:22
0
\$\begingroup\$

I re-wrote my script with a for loop, which I should have done from the beginning. However there's still a lot of repetition so my gut is telling me it could be better.

However, I think its much more readable this time and I no longer get extraneous page numbers at the end.

pages =             float(input("How many total pages is the book? "))
booklets =          pages / 32
pages_per_booklet = pages / booklets
last_booklet =      pages % 32
k = 1
booklet = []

for bklts in range(1, int(booklets + 1)):
    n = int(pages_per_booklet * k)
    f = 1 + (int(pages_per_booklet) * (k-1))    
    booklet.extend(make_booklet(f, n))
    k += 1
    if (len(booklet) + int(last_booklet) == pages):
        n = int(pages)
        f = 1 + int(pages - last_booklet)
        booklet.extend(make_booklet(f, n))

def make_booklet(f, n):
    booklet = []
    while n > f:
        booklet.extend([n, f, f+1, n-1])
        f += 2
        n -= 2
    return booklet

print(booklet)
\$\endgroup\$
  • \$\begingroup\$ The reason the range of the for loop is from 1 to x+1 is because i was originally planning on using the index and range for the calculations in the loop, but then changed my mind \$\endgroup\$ – qazwsx598 Mar 11 '18 at 17:50
  • 1
    \$\begingroup\$ Your code does not print anything when I input 20. I highly suggest writing tests before trying to rewrite your code . See my answer. \$\endgroup\$ – SylvainD Apr 10 '18 at 8:00
  • \$\begingroup\$ it works after I move the def make_booklet() function definition to line 8 and insert if pages < 32: booklets = 1 to line 3. I'm not entirely sure why I didn't have problems before, but I did test it and had it working before I posted it here. I could have gotten excited and copied the wrong version.. There are still a few minor bugs when pages isn't a multiple of 4 where the last list item is either missing or duplicated. I don't know how to fix that \$\endgroup\$ – qazwsx598 Apr 11 '18 at 0:35

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.