Radomir Dopieralski
Red Hat 2014
Wrong:
from my_lib import *
do_something()
Better:
from my_lib import do_something
do_something()
Even better:
import my_lib
my_lib.do_something()
Wrong:
from library_a import utils as library_a_utils
from library_b import utils as library_b_utils
library_a_utils.do_something()
library_b_utils.yadda_yadda()
Better:
import library_a.utils
import library_b.utils
library_a.utils.do_something()
library_b.utils.yadda_yadda()
Wrong:
from your_project import Foo
from your_project import Bar
assert Foo
assert Bar
Better:
from your_project import Foo
from your_project import Bar
__all__ = ['Foo', 'Bar']
Wrong:
def make_html_link(href, label, class):
return '<a href="%s" class="%s">%s</a>' % (
html_escape(href),
html_escape(class),
html_escape(label),
)
Better:
def make_html_link(href, label, class_):
...
def make_html_link(href, label, css_class):
...
Commonly shadowed names: class, map, id, _, all, input, list, max, min, next, object, type, dir
Wrong:
def do_something():
from something import do_it
do_it()
Better:
import something
def do_something():
something.do_it()
>>> class EqualToEverything(object):
... def __eq__(self, to_what):
... return True
...
>>> EqualToEverything() == None
True
>>> EqualToEverything() is None
False
>>> EqualToEverything() is not None
True
Wrong:
class C(object):
def __eq__(self, other):
...
C() != C()
Better:
class C(object):
def __eq__(self, other):
...
def __ne__(self, other):
return not self.__eq__(other)
Better:
import functools
@functools.total_ordering
class C(object):
...
Variables are not containers – they are just names attached to objects. You can have two names attached to the same object.
>>> x = [1]
>>> y = x
>>> x is y
True
>>> y = [1]
>>> x is y
False
When an object is of mutable type, it is modified in place. That means that all names that refer to it will now refer to the new value.
>>> list1 = [1]
>>> list2 = list1 # Doesn't make a copy!
>>> list1.append(2)
>>> list1
[1, 2]
>>> list2
[1, 2]
>>> list2 = [3, 4]
>>> list1.append(5)
>>> list1
[1, 2, 5]
>>> list2
[3, 4]
Function arguments are always passed by reference, never by value.
Wrong:
>>> def empty_list(what):
... what = []
>>> list1 = [1, 2, 3]
>>> empty_list(list1)
>>> list1
[1, 2, 3]
Good:
>>> def empty_list(what):
... what[:] = []
>>> list1 = [1, 2, 3]
>>> empty_list(list1)
>>> list1
[]
Wrong:
return answer and "yes" or "no"
return ("yes", "no")[answer]
return {True: "yes", False: "no"}[answer]
Better:
return "yes" if answer else "no"
Best:
def format_answer(answer):
if answer:
return "yes"
return "no"
return format_answer(answer)
Wrong:
if x == True: ...
if (x == True) == True: ...
if ((x == True) == True) == True: ...
...
Better:
if x: ...
Fine:
>>> if x > 0 and x < 5: ...
Better:
>>> if 0 < x < 5: ...
By the way:
>>> None < float('-inf') < 0 < float('+inf')
True
Fine:
if not(x or not y): ...
Better:
if not x and y: ...
Wrong:
if ~x & y: ...
Fine:
>>> a = 0
>>> b = a
>>> c = a
Better:
>>> a = b = c = 0
Caution!
>>> a = b = c = []
>>> a.append(1)
>>> a, b, c
([1], [1], [1])
>>> a, b, c = 1, 2, 3
>>> a, b, c
(1, 2, 3)
>>> FOO, BAR, BAZ = range(3)
>>> BAR
1
>>> head, tail = 'a, b, c'.split(',', 1)
>>> head, tail
('a', ' b, c')
>>> a, b = 1, 2
>>> b, a = a, b
>>> a, b
(2, 1)
>>> a, b = 1, 2, 3
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ValueError: too many values to unpack
Wrong:
tuple1 = (1, 2)
tuple2 = (1)
Better:
tuple1 = 1, 2
tuple2 = 1,
tuple1 = (
1,
2,
)
tuple2 = (
1,
)
Wrong:
if my_dict.has_key('foo'):
...
Better:
if 'foo' in my_dict:
...
Wrong:
if 'foo' in my_dict:
return my_dict['foo']
else:
return 'bar'
Better:
try:
return my_dict['foo']
except KeyError:
return 'bar'
return my_dict.get('foo', 'bar')
Wrong:
for key in my_dict.keys():
do_something(key, my_dict[key])
Better:
for key, value in my_dict.iteritems():
do_something(key, value)
Fine:
for key, value in other_dict.iteritems():
my_dict[key] = value
Better:
my_dict.update(other_dict)
Wrong:
my_dict.update({
'single_key': 3,
})
Better:
my_dict['single_key'] = value
Fine:
foo = my_dict['foo']
del my_dict['foo']
Better:
foo = my_dict.pop('foo')
Wrong:
>>> f = open('filename')
>>> data = f.read()
>>> f.close
<built-in method close of file object at 0xb786a020>
Better:
>>> f = open('filename')
>>> data = f.read()
>>> f.close()
Wrong:
>>> def append_one(where=[]):
... where.append(1)
... return where
>>> append_one()
[1]
>>> append_one()
[1, 1]
Better:
>>> def append_one(where=None):
... if where is None:
... where = []
... where.append(1)
... return where
Wrong:
def function(**kwargs):
foo = kwargs['foo']
...
Better:
def function(foo):
...
Wrong:
def decorate(function):
def decorated(*args, **kwargs):
...
return function(*args, **kwargs)
return decorated
Better:
import functools
def decorate(function):
@functools.wraps(function)
def decorated(*args, **kwargs):
...
return function(*args, **kwargs)
return decorated
Fine:
class C(object):
def __init__(self):
self._x = None
def getx(self):
return self._x
def setx(self, value):
self._x = value
def delx(self):
del self._x
x = property(getx, setx, delx)
Better:
class C(object):
def __init__(self):
self._x = None
@property
def x(self):
return self._x
@x.setter
def x(self, value):
self._x = value
@x.deleter
def x(self):
del self._x
>>> hello = "Hello world!"
>>> hello[0]
'H'
>>> hello[0:5]
'Hello'
>>> hello[-1]
'!'
>>> hello[1:-1]
'ello world'
>>> hello[:5] + hello[5:]
'Hello world!'
>>> hello[::2]
'Hlowrd'
>>> hello[::-1]
'!dlrow olleH'
>>> a = [1, 2, 3, 4]
>>> a[1:3] = [5, 6, 7, 8]
>>> a
[1, 5, 6, 7, 8, 4]
>>> a[::2] = [0, 0, 0]
>>> a
[0, 5, 0, 7, 0, 4]
>>> s = list('abc')
>>> s[1:2] = 'ABC'
>>> ''.join(s)
'aABCc'
Fine:
result = []
for item in items:
if is_good(item):
result.append(wrap(item))
Better:
result = [wrap(item) for item in items if is_good(item)]
Wrong:
result = items.sort()
Better:
result = sorted(items)
Better:
items.sort()
result = items
Wrong:
result = 0
for item in items:
result += item
Better:
result = sum(items)
There are also max
and min
.
Wrong:
result = max([item[1] for item in items])
Better:
def get_second(item):
return item[1]
result = max(items, key=get_second)
Better:
result = max(items, key=lambda item: item[1])
Can be used with max
, min
, sort
.
Notice that functions can be passed as arguments!
Wrong:
items = ['a', 'b', 'c']
for i in xrange(len(items)):
print items[i]
Good:
for item in ['a', 'b', 'c']:
print item
Wrong:
result = []
for i in range(min(len(first), len(second))):
result.append((first[i], second[i]))
Good:
result = zip(first, second)
Example:
>>> zip([1, 2, 3], [4, 5, 6]):
[(1, 4), (2, 5), (3, 6)]
Wrong:
for i in range(len(items)):
print i, items[i]
Good:
for i, item in enumerate(items):
print i, item
Wrong:
found = None
for item in items:
if is_good(item):
found = item
if found is None:
print "not found"
else:
print "found %r" % found
Good:
for item in items:
if is_good(item):
print "found %r" % item
break
else:
print "not found"
Wrong:
def get_first_or_default(query, default=None):
try:
return list(query)[0]
except IndexError:
return default
Fine:
def get_first_or_default(query, default=None):
for item in query:
return item
return default
Better:
def get_first_or_default(query, default=None):
return next(iter(query), default)
Wrong:
try:
...
except:
...
Better:
try:
...
except Exception as e:
# Do something with e, don't just pass.
Wrong:
>>> foo = {'bar': 2}
>>> try:
... print fooo['bar']
>>> except Exception:
... print "there is no 'bar'"
Better:
>>> foo = {'bar': 2}
>>> try:
... print fooo['bar']
>>> except KeyError:
... print "there is no 'bar"
Wrong:
>>> foo = {'bar': 2}
>>> try:
... print foo['bar']
... baz = foo['bar'] + foo['foo']
>>> except KeyError:
... print "there is no 'bar'"
Good:
>>> foo = {'bar': 2}
>>> try:
... print foo['bar']
>>> except KeyError:
... print "there is no 'bar'"
>>> else:
... baz = foo['bar'] + foo['foo']
Wrong:
foo = {'bar': 2}
if 'bar' in foo:
bar = foo['bar']
else:
bar = 0
bar = foo['bar'] if foo.has_key('foo') else 0
Better:
foo = {'bar': 2}
try:
bar = foo['bar']
except KeyError:
bar = 0
bar = foo.get('bar', 0)
Fine:
f = open('filename')
try:
data = f.read()
finally:
f.close()
Better:
from __future__ import with_statement
with open('filename') as f:
data = f.read()
Fine:
with X() as x:
with Y() as y:
do_something(x, y)
Better:
with X() as x, Y() as y:
do_something(x, y)
Older pythons:
import contextlib
with contextlib.nested(X(), Y()) as x, y:
do_something(x, y)
Wrong:
try:
f = open('filename')
except OSError, IOError:
...
It's the same as:
try:
f = open('filename')
except OSError as IOError:
...
Better:
try:
f = open('filename')
except (OSError, IOError):
...
import errno
try:
f = open('filename')
except IOError, e:
if e.errno == errno.ENOENT:
print "No such file"
else:
raise
except OSError:
print "System error"
Wrong:
if broken:
raise Exception("Something went wrong.")
Better:
class BrokenException(Exception):
"""Indicates that our stuff is broken."""
...
if broken:
raise BrokenException("Something went wrong in %r." % details)
Wrong:
if broken:
RuntimeError()
Better:
if broken:
raise RuntimeError()
Wrong:
if broken:
raise RuntimeError
Better:
if broken:
raise RuntimeError()
Wrong:
class C:
...
Better:
class C(object):
...
Wrong:
class A(object):
def foo(self, arg):
return ...
class B(A):
def foo(self, arg):
...
return A.foo(self, arg)
Better:
class B(A):
def foo(self, arg):
...
return super(B, self).foo(arg)
Wrong:
value = my_object.__dict__[name]
value = my_object.__getattr__(name)
value = my_object.__getattribute__(name)
value = eval('my_object.%s' % name)
exec('value = my_object.%s' % name)
Better:
value = getattr(my_object, name)
Wrong:
if hasattr(my_object, name):
value = getattr(my_object, name)
else:
value = None
Better:
value = getattr(my_object, name, None)
Wrong:
return getattr(self, '_cached', self.some_costly_function())
Better:
try:
return getattr(self, '_cached')
except AttributeError:
return self.some_costly_function()
Wrong:
if not isinstance(parameter, list):
raise TypeError("Please provide a list.")
Better:
iter(parameter)
Fine:
import collections
if not isinstance(parameter, collections.Iterable):
raise TypeError("Please provide an interable.")
Wrong:
if x == True: ...
if x != []: ...
if x == []: ...
if x == {}: ...
if len(x) == 0: ...
if x == '': ...
if x:
return True
else:
return False
Better:
if x:
if not x:
return x
Fine:
return bool(x)
Wrong:
class C(object):
def __init__(self, source):
if isinstance(source, basestring):
source = source.split(',')
elif isinstance(source, list):
source = source
elif isinstance(source, dict):
source = source.values()
...
Better:
class C(object):
def __init__(self, source_list):
...
@classmethod
def from_string(cls, source_string):
return cls(source_string.split(','))
@classmethod
def from_dict(cls, source_dict):
return cls(source_dict.values())
Wrong:
class A(object):
...
class B(A):
def __new__(cls, *args, **kwargs):
if ...:
return A.__new__(*args, **kwargs)
return super(B, cls).__new__(*args, **kwargs)
Better:
def make_A_or_B(*args, **kwargs):
if ...:
return A(*args, **kwargs)
return B(*args, **kwargs)
Wrong:
class Resource(object):
def __init__(self):
self.resource = get_resource()
def __del__(self):
release_resource(self.resource)
resource = Resource()
...
Better:
class Resource(object):
def __enter__(self):
self.resource = get_resource()
return self
def __exit__(self, exc_type, exc_value, traceback):
release_resource(self.resource)
with Resource() as resource:
...
Wrong:
x = input("Enter x:")
y = input("Enter y:")
print "x + y = %d" % (x + y)
Better:
x = int(raw_input("Enter x:"))
y = int(raw_input("Enter y:"))
print "x + y = %s" % (x + y)
Fine:
import sys
if len(sys.args) > 1:
with open(sys.args[1]) as f:
for line in f:
do_something(line)
else:
for line in sys.stdin:
do_something(line)
Better:
import fileinput
for line in fileinput.input():
do_something(line)
Fine:
score_list = [
('alice', 5),
('bob', 1),
('bob', 3),
('alice', 2),
...
]
scores_by_name = {}
for name, score in score_list:
scores = scores_by_name.get(name, [])
scores.append(score)
scores_by_name[name] = scores
Better:
for name, score in score_list:
scores_by_name.setdefault(name, []).append(score)
Better:
import collections
score_list = [
('alice', 5),
('bob', 1),
('bob', 3),
('alice', 2),
...
]
scores_by_name = collections.defaultdict(list)
for name, score in score_list:
scores_by_name[name].append(score)
Fine:
score_list = [
('alice', 5),
('bob', 1),
('bob', 3),
('alice', 2),
...
]
total_scores_by_name = {}
for name, score in score_list:
total_scores_by_name[name] = total_scores_by_name.get(name, 0) + score
Better:
import collections
total_scores_by_name = collections.Counter()
for name, score in score_list:
total_scores_by_name[name] += score
Wrong:
queue = [root]
while queue:
item = queue.pop(0)
...
for child in item.children:
queue.append(child)
Better:
import collections
queue = collections.deque()
while queue:
item = queue.popleft()
...
for child in item.children:
queue.append(child)