daviddoji/professional-programming
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Remove Python antipatterns, moved to Python repo

Browse Source
This commit is contained in:
Charles-Axel Dein 2020-09-16 13:55:06 +02:00
parent c1878d9826
commit e24a5a7973
1 changed files with 0 additions and 248 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

248
antipatterns/python-antipatterns.md
View File

@ -1,248 +0,0 @@
<!-- START doctoc generated TOC please keep comment here to allow auto update -->
<!-- DON'T EDIT THIS SECTION, INSTEAD RE-RUN doctoc TO UPDATE -->
## Table of Contents
- [Python Antipatterns](#python-antipatterns)
- [Redundant type checking](#redundant-type-checking)
- [Restricting version in setup.py dependencies](#restricting-version-in-setuppy-dependencies)
- [Unwieldy if... else instead of dict](#unwieldy-if-else-instead-of-dict)
- [Overreliance on kwargs](#overreliance-on-kwargs)
- [Overreliance on list/dict comprehensions](#overreliance-on-listdict-comprehensions)
- [Mutable default arguments](#mutable-default-arguments)
- [Using `is` to compare objects](#using-is-to-compare-objects)
- [Instantiating exception with a dict](#instantiating-exception-with-a-dict)
- [Not strictly pinning all packages](#not-strictly-pinning-all-packages)
- [Reference](#reference)
<!-- END doctoc generated TOC please keep comment here to allow auto update -->
# Python Antipatterns
## Redundant type checking
Bad:
```python
def toast(bread):
if bread is None:
raise TypeError('Need bread to toast.')
if bread.is_toastable:
toaster.toast(bread)
```
In this case, checking against `None` is totally useless because in the next
line, `bread.is_toastable` would raise `AttributeError: 'NoneType' object has no attribute 'is_toastable'`. This is not a general rule, but in this case
I would definitely argue that adding the type checks hurts readability and adds
very little value to the function.
Good:
```python
def toast(bread):
if bread.is_toastable:
toaster.toast(bread)
```
## Restricting version in setup.py dependencies
Read those articles first:
- [setup.py vs.
requirements.txt](https://caremad.io/2013/07/setup-vs-requirement/)
- [Pin Your Packages](http://nvie.com/posts/pin-your-packages/)
- [Better Package Management](http://nvie.com/posts/better-package-management/)
**Summary: The main point is that `setup.py` should not specify explicit version
requirements (good: `flask`, bad: `flask==1.1.1`).**
In a few words, if library `lib1` requires `flask==1.1.1` and library `lib2`
requires `flask==1.1.2`, then you'll have a conflict and won't be able to use
them both in application `app`.
Yet in 99.999% of the cases, you don't need a specific version of flask, so:
- `lib1` should just require `flask` in `setup.py` (no version specified, not
even with inequality operators: `flask<=2` is bad for instance)
- `lib2` should just require `flask` in `setup.py` (same)
`app` will be happy using `lib1` and `lib2` with whatever version of `flask` it
wants.
`app`'s `requirements.txt` should be as specific as possible, ideally
strictly pinning (`==`) every single dependency. This way the app's stability
will be very predictable, because always the same packages version will be
installed.
Usually apps only use `requirements.txt`, not `setup.py`, because `pip install -r requirements.txt` is used when deploying.
The only exception for pinning a dependency in a library is in case of a known
incompatibility, but again this should be a very temporary move, because that
will prevent people from upgrading.
Ruby has a pretty similar dichotomy with [Gemspec and
gemfile](http://yehudakatz.com/2010/12/16/clarifying-the-roles-of-the-gemspec-and-gemfile/).
## Unwieldy if... else instead of dict
Bad:
```python
import operator as op
def get_operator(value):
"""Return operator function based on string.
e.g. ``get_operator('+')`` returns a function which takes two arguments
and return the sum of them.
"""
if value == '+':
return op.add
elif value == '-':
return op.sub
elif value == '*':
return op.mul
elif value == '/':
return op.div
else:
raise ValueError('Unknown operator %s' % value)
```
Note: the operator module is a standard library modules that defines base
operator functions. For instance `operator.add(1, 1) == 2`.
This huge switch-like expression will soon become really difficult to read and
maintain. A more pythonic way is to use a dict to store the mapping.
Another reason is that to get 100% line and branch coverage, you will have to
create as many tests as you have mappings. Yet you could consider that the
value to operator mapping is part of the codebase's configuration, not its
behavior, and thus shouldn't be tested.
Good:
```python
import operator as op
OPERATORS = {
'+': op.add,
'-': op.sub,
'*': op.mul,
'/': op.div,
}
def get_operator(value):
"""Return operator function based on string."""
operator = OPERATORS.get(value)
if operator:
return operator
else:
raise ValueError('Unknown operator %s' % value)
```
## Overreliance on kwargs
TODO
## Overreliance on list/dict comprehensions
TODO
## Mutable default arguments
TODO
## Using `is` to compare objects
TODO
[Why you should almost never use “is” in
Python](http://blog.lerner.co.il/why-you-should-almost-never-use-is-in-python/)
## Instantiating exception with a dict
Example:
```python
def main():
raise Exception({'msg': 'This is not a toaster', 'level': 'error'})
```
Why is this an antipattern? Exception are meant to be read by human beings.
Thus, their first argument should be a human-readable error message, like this:
```python
class NotAToasterException(Exception):
pass
def main():
raise NotAToasterException('This is not a toaster')
```
Most tools expect this, most importantly
[Sentry](https://getsentry.com/welcome/) which is a tool used to get alerts
when exception are raised in production. An `Exception`'s message should be
short so that it can be displayed on a single line.
If you need to attach custom metadata to an exception, the proper way is to
have a custom constructor:
```python
class NotAToasterException(Exception):
def __init__(self, message, level):
super(NotAToasterException, self).__init__(message)
self.message = message
self.level = level
def main():
raise NotAToasterException('This is not a toaster', 'error')
```
## Not strictly pinning all packages
Example of bad `requirements.txt` file:
```
sqlalchemy
flask
```
Another example of a bad `requirements.txt` file:
```
sqlalchemy>=0.9
flask>0.10
```
Good:
```
Flask==0.10.1
Jinja2==2.8
MarkupSafe==0.23
SQLAlchemy==1.0.12
Werkzeug==0.11.4
itsdangerous==0.24
```
This ensures that there's absolutely no ambiguity as to which package will be installed on production. If you forget to mention even a single package, you will perhaps have a different version on your testing/dev environment, and in your production environment.
The proper way to update a package and its dependency is to use another tool, for instance [pip-tools](https://github.com/nvie/pip-tools). If you have multiple applications and you want to mass update a package, then you'll have to write a script to do so. Keep things simple and explicit, then use scripts on top of it to instrument your processes.
**Reference**
- [Pin Your Packages](http://nvie.com/posts/pin-your-packages/)
# Reference
- [Pythonic Pitfalls](http://nafiulis.me/potential-pythonic-pitfalls.html)
- [Python Patterns](https://github.com/faif/python-patterns)
- [The Little Book of Python
Anti-Patterns](http://docs.quantifiedcode.com/python-anti-patterns/)
- [How to make mistakes in
Python](http://www.oreilly.com/programming/free/files/how-to-make-mistakes-in-python.pdf)
G