Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code review! #1

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Code review! #1

wants to merge 16 commits into from

Conversation

akaptur
Copy link

@akaptur akaptur commented Oct 11, 2013

Some thoughts & suggestions. Some of these are half-formed (i.e. I broke your code :), but point in the direction of changes I would make. I'd be happy to talk about these or take another look after you finish your numpy refactoring.

... to better highlight the small differences between the two functions
You have well-documented functions here: make the docs accessible by formatting them with triply-quoted strings.  (See, eg. my_function.func_doc).
It's not generally pythonic to have a "getter" like this - you usually want to just access the object's attributes directly.  It looks like you're signalling to the end user that the width and height aren't part of the object's public API.  I think it's more clean to skip this (and in fact, I'd change the names from _width and _height to width and height).  Others may disagree :)
We found ourselves asking which way transposed was - this naming makes that explicit.
Feel free to be verbose in your variable names in pursuit of readability :)
Refactors edge and distance calculation to avoid confusing (to me) transposition.  This commit isn't quite complete/working, but it gives an idea of the direction I'd suggest going.

By the way, it's possible to swap two variables in python in one line:
b, a = a, b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant