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

equal_attributes function optimization #1477

Merged
merged 1 commit into from Sep 7, 2015
Merged

equal_attributes function optimization #1477

merged 1 commit into from Sep 7, 2015

Conversation

@mlyundin
Copy link
Contributor

@mlyundin mlyundin commented Sep 3, 2015

Please review. From my point of view now function looks better.

@codecov-io
Copy link

@codecov-io codecov-io commented Sep 3, 2015

Current coverage is 82.37%

Merging #1477 into master will increase coverage by +0.04% as of bfa9d41

Powered by Codecov. Updated on successful CI builds.

@@ -254,25 +254,22 @@ def get_spec(func):
def equal_attributes(obj1, obj2, attributes):
"""Compare two objects attributes"""
# not attributes given return False by default
if not attributes:
return False

This comment has been minimized.

@kmike

kmike Sep 3, 2015
Member

This is a common style to write functions: first we check special cases, and then we write the main function body. It allows to remove unnecessary nesting and makes code easier to read to people familiar with this convention. I.e. in existing implementation it reads like the main thing function is doing is checking each attribute (for attr in attributes:); in your version this loop is in an indented block after some statements. It may be a personal preference, but I like the existing way to write it more.

if callable(attr):
if not attr(obj1) == attr(obj2):
return False
res = False

This comment has been minimized.

@kmike

kmike Sep 3, 2015
Member

I don't think Dijkstra's style is a good fit to Python; http://anthonysteele.co.uk/the-single-exit-point-law provide some arguments I agree with. IMHO using return is fine: it is shorter and easier to read. In you implementation developer have to check if res is changed later or not to understand what is code doing; return False allows a more local reasoning - less things to keep in mind.

for attr in attributes:
# support callables like itemgetter
if callable(attr):
if attr(obj1) != attr(obj2):

This comment has been minimized.

@kmike

kmike Sep 3, 2015
Member

+1 to change not attr(obj1) == attr(obj2) to this version.

@mlyundin mlyundin force-pushed the mlyundin:master branch 2 times, most recently from 9c8f99c to fc49959 Sep 3, 2015
@mlyundin
Copy link
Contributor Author

@mlyundin mlyundin commented Sep 4, 2015

I have applied comments. Please review once again.

return False
# compare object attributes
if not getattr(obj1, attr) == getattr(obj2, attr):
elif getattr(obj1, attr, temp1) != getattr(obj2, attr, temp2):
return False

This comment has been minimized.

@nramirezuy

nramirezuy Sep 4, 2015
Contributor

fix indentation in this line, plz.

@mlyundin mlyundin force-pushed the mlyundin:master branch from fc49959 to d022d3c Sep 4, 2015
dangra added a commit that referenced this pull request Sep 7, 2015
equal_attributes function optimization
@dangra dangra merged commit 6490cb5 into scrapy:master Sep 7, 2015
2 checks passed
2 checks passed
codecov/patch 100.00% of diff hit (target 100.00%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants