amontalenti / elements-of-python-style

Goes beyond PEP8 to discuss what makes Python code feel great. A Strunk & White for Python.
3.44k stars 260 forks source link

Make improvements to dedupe(), add new rule on in-place modifications #19

Closed amontalenti closed 8 years ago

amontalenti commented 8 years ago

This PR makes the dedupe() example clearer by making the return values of the various implementations more similar. This was raised in PR #13.

This also clarifies the distinction of declarative and imperative in an earlier rule, as raised in issue #10, by using a final "best" rewrite of this function as an example of that.

Finally, this PR adds a new rule. It clarifies the point this example was trying to make by illustrating a usage bug with impure functions related to in-place modification. The new rule is that if you must do in-place modifications, you should return None from your function, rather than returning the modified structure.

j6k4m8 commented 8 years ago

Love it. Makes this much, much clearer, and addresses the functions-of-same-return-value concern I had.

veprbl commented 8 years ago

:+1:

But there is still a problem that "good" and "bad" codes are not equivalent. "bad" one does retain element order, which may be important in some cases. The comparison is not entirely fair.

amontalenti commented 8 years ago

@veprbl I think that's acceptable because the "bad" code doesn't advertise this constraint, so it isn't actually part of the contract. It's an implementation detail. Thanks for the feedback.

veprbl commented 8 years ago

This got very convoluted for illustration of the concept. Maybe it's worth considering something simple like:

def reverse_sort(items):
    items.sort(cmp=lambda x,y: cmp(y, x))
    return items

vs

def reverse_sort(items):
    items = list(items)
    items.sort(cmp=lambda x,y: cmp(y, x))
    return items
amontalenti commented 8 years ago

@veprbl You have a point. I am going to let this PR sit for awhile. I agree that it is a tad longer and more convoluted than I'd like.

veprbl commented 8 years ago

Sorry, I have one more idea on the topic:

The question of purity raises here mostly due to python's property that it passes objects lists, dicts and class instances by reference. The user might get some sad hours debugging:

def __init__(self, items): # misery propagates further up
    self.uniq_items = dedupe(items)
    self.orig_size = len(items) # unexpected result

This might be worth mentioning.

And set(), of course, deserves a praise of its own, maybe in a section like "Prefer simple argument and return types". I don't see how one can rewrite dedupe to be impure and to lose item ordering at the same time without making the code unrealistically bad.

amontalenti commented 8 years ago

Discussion should move to #23.