#19890: Improve standardization
-------------------------------------+-------------------------------------
Reporter: elixyre | Owner:
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-7.0
Component: combinatorics | Resolution:
Keywords: | Merged in:
Authors: Jean-Baptiste | Reviewers: Travis Scrimshaw
Priez | Work issues:
Report Upstream: N/A | Commit:
Branch: | 798a3fb4e5809e9aafe9c7ea0b4d13bc0b051788
u/elixyre/standardization | Stopgaps:
Dependencies: |
-------------------------------------+-------------------------------------
Comment (by tscrim):
Here are the changes I'd make:
{{{#!diff
- def merge(shelves, i=0):
+ def merge(shelves, i):
- if len(shelves) == 0:
+ if not shelves:
return
+ n = len(shelves)
- if len(shelves) == 1:
+ if n == 1:
yield (shelves[0], i)
return
- n = len(shelves)
- m = int(n / 2)
+ m = int(n // 2)
L, R = merge(shelves[:m], i), merge(shelves[m:], i+m)
aL, aR = L.next(), R.next()
- j, k = 1, 1
+ j, k = 0, 0
while True:
if aL[0] <= aR[0]:
yield aL
j += 1
- if j == m+1: break
- else: aL = L.next()
+ if j == m:
+ break
+ aL = L.next()
else:
yield aR
k += 1
- if k == n - m + 1: break
- else: aR = R.next()
+ if k == n - m:
+ break
+ aR = R.next()
- if j == m+1:
+ if j == m:
- it = R
yield aR
+ for a in R:
+ yield a
else:
- it = L
yield aL
+ for a in L:
+ yield a
- for a in it:
- yield a
}}}
Here is my reasoning:
- I do not think you should have a default for `i` since you almost always
call it with an argument. I would just make the one place where you use
the default argument (the `for` loop in the main function) explicitly pass
`0`.
- The `if not shelves` is faster than calling `len` and comparing to `0`.
- Moving the `n = len(shelves)` call up typically avoids an extra call to
`len` and variable allocation is faster than a function call.
- We should use `//` for integer division.
- If we start at 0, we avoid a number of unnecessary `+1`'s and keeps it
more in line with standard python code.
- Standard Python formatting should only have `if` (and `else`) statements
on one line.
- By not having the `it` variable and the distinct `for` loops, IMO it
makes the code more clear as there are less variables to chase around (and
it has the same length).
If you agree and make these changes, you can set a positive review on my
behalf.
--
Ticket URL: <http://trac.sagemath.org/ticket/19890#comment:3>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at https://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
