3
votes
\$\begingroup\$

What are the most common improvements of Python code you've applied or seen applied (to you or someone else) here on codereview?

\$\endgroup\$
6
  • \$\begingroup\$ It's not clear what you asking. Is this question meant for reviewers? \$\endgroup\$
    – tshepang
    Commented Mar 26, 2012 at 9:41
  • \$\begingroup\$ @Tshepang: Yes, but not them only :) I put that last "applied here on codereview" just to keep the answers focused and on topic. I'll update the question to made it more clear. \$\endgroup\$
    – Rik Poggi
    Commented Mar 26, 2012 at 9:49
  • \$\begingroup\$ This question is also related to 2 others you have asked here on Meta (first, second). Maybe you also wanna mention that fact? \$\endgroup\$
    – tshepang
    Commented Mar 26, 2012 at 9:59
  • \$\begingroup\$ @Tshepang: No I didn't wanted to mention that. \$\endgroup\$
    – Rik Poggi
    Commented Mar 26, 2012 at 10:06
  • \$\begingroup\$ ...and why not? \$\endgroup\$
    – tshepang
    Commented Mar 26, 2012 at 10:09
  • \$\begingroup\$ @Tshepang: Because I didn't wanted to "scatter" the attention on why the question is there (your first link), and at the second link there seem to be nothing interesting: very little up/down vote partecipation, and the only upvoted answer it just tells to open this question. \$\endgroup\$
    – Rik Poggi
    Commented Mar 26, 2012 at 10:14

7 Answers 7

6
votes
\$\begingroup\$

Loops

In Python you should rarely use while loops or for x in range(n) loops. Python has a wide variety of tools like zip, enumerate, itertools.* to iterate over pretty much anything with a for loop and an iterator.

Don't write:

for x in range(len(data)):
    print(data[x])

instead write:

for item in data:
    print(item)

Don't write:

for x in range(len(data)):
    print((x + 1, data[x]))

Write:

for index, item in enumerate(data, start=1):
    print((index, item))

Don't write:

for x in range(len(data1)):
    print(data1[x] + data2[x])

Write:

for item1, item2 in zip(data1, data2):
    print(item1 + item2)

(In Python 2, you'd use from future_builtins import zip to get the version of zip that returns an iterator instead of a list.)

\$\endgroup\$
5
  • \$\begingroup\$ It should be good to link to the doc, for zip etc.. And maybe it could also be added a warning at then to the py3k zip doc. \$\endgroup\$
    – Rik Poggi
    Commented Mar 28, 2012 at 9:58
  • 1
    \$\begingroup\$ I'd love to see a concrete example. \$\endgroup\$ Commented Mar 30, 2012 at 12:09
  • \$\begingroup\$ In Python 2, the last example (with zip) allocates a list, which might not always be a good idea. \$\endgroup\$ Commented Sep 14, 2012 at 19:02
  • \$\begingroup\$ @GarethRees, feel free to edit of the answers on this page to improve them. \$\endgroup\$
    – Winston Ewert Mod
    Commented Sep 14, 2012 at 20:59
  • \$\begingroup\$ Thank you. But I don't yet have enough reputation on Code Review to edit answers. \$\endgroup\$ Commented Sep 15, 2012 at 15:42
5
votes
\$\begingroup\$

file handling

Instead of closing files yourself, have the with statement do it for you:

with open('filename') as filehandler:
    do_stuff(filehandler)

and not:

filehandler = open('filename')
do_stuff(filehandler)
filehandler.close()

Beyond saving you to have to close the file manually, the with statement also ensures the file will be closed even though exceptions rise during execution of do_stuff() function. That is, you get to avoid this ugliness:

filehandler = open('filename')
try:
    do_stuff(filehandler)
finally:
    filehandler.close()
\$\endgroup\$
1
  • 1
    \$\begingroup\$ I would apply this to more than just file handles. I use the with statement for StringIO and handling network connections as well. Anything that can be closed, and many things that can't are fair game. contextlib.closing for the win! \$\endgroup\$
    – kojiro
    Commented May 18, 2012 at 19:59
4
votes
\$\begingroup\$

PEP 8 -- Style Guide for Python Code

To follow a standard coding style is very important.

It's even more important if you want other people to look at your code.

By following the official coding style guide your code will be more readable and it will be way more easy for others programmers to understand it. You should really read PEP8, it may seem long, but it's just a bunch of rules, the sooner you'll start to follow them, the better will be.

Anyway this is an attemp to summarize the most "overlooked" ones:

1) Indention

Use 4 spaces per indentation level and never mix tabs and spaces.

2) Maximum Line Length

Limit all lines to a maximum of 79 characters. 80 is okey too.

3) Leave a space around the operators:

Yes:

i = 10
i = i + 1
i += 1

No:

i=10
i=i+1
i+=1

4) Documention strings

Conventions for writing good documentation strings (a.k.a. "docstrings") are immortalized in PEP 257, so take a look at that one too. Anyway in brief:

  • One-line Docstrings: One-liners are for really obvious cases. They should really fit on one line. For example:

    def add(a, b): """Return a + b, for a and b numbers..""" return a + b

  • Multi-line Docstrings: Multi-line docstrings consist of a summary line just like a one-line docstring, followed by a blank line, followed by a more elaborate description.

    The docstring for a function or method should summarize its behavior and document its arguments, return value(s), side effects, exceptions raised, and restrictions on when it can be called (all if applicable). Optional arguments should be indicated. It should be documented whether keyword arguments are part of the interface.

    Example:

    def fetch_bigtable_rows(big_table, keys, other_silly_variable=None):
        """Fetches rows from a Bigtable.
    
        Retrieves rows pertaining to the given keys from the Table instance
        represented by big_table.  Silly things may happen if
        other_silly_variable is not None.
    
        """
        # code starts from here
    

I don't want to make this section too complex, so other than PEP 257 you can find more in the google guidelines, at this example of pypi project, and this SO question: Docstrings - one line vs multiple line.

Note: If a docstring become too complex to write/read/follow it's usually a sign that the function need to be refactored in two or more simple ones.

5) Naming Conventions

You should read them all, anyway here's a small summary of the most common ones:

  • "normal" variables and functions: all_lowercase_with_underscore.
    Dumb and verbose example:

    def my_function(first_arg, second_arg):
        sum_of_args = first_arg + second_arg
        # ...
    
  • Class Names: CamelCase. Example:

    class MyClass:
        # ...
    
  • Constants: ALL_CAPS.
    Constants are usually defined on a module level and written in all capital letters with underscores separating words. Examples:

    MAX_OVERFLOW = 100
    TOTAL = 70
    
\$\endgroup\$
2
  • 1
    \$\begingroup\$ I've started with PEP8 beacuse the lack of following this coding style guide happens very often and divert the attention from the "real" review. \$\endgroup\$
    – Rik Poggi
    Commented Mar 26, 2012 at 11:50
  • \$\begingroup\$ I was hoping you wouldn't dump the PEP-8 here, but instead cover specific issues it addresses in separate Answers. \$\endgroup\$
    – tshepang
    Commented May 18, 2012 at 20:37
4
votes
\$\begingroup\$

Have as little code as possible in the global namespace. Most logic should be in functions, classes, or methods. This helps prevent polluting the global namespace and will run faster.

On a related note, the main function should look something like this:

def main(list_of_stuff):
    ...

if __name__ == "__main__":
    main(sys.argv)

This is to avoid the globally-defined things, e.g. sys.argv in this case, to be executed should the module be called from another Python module. In our example, main() gets its list_of_stuff from the command line if ran directly, but if called from another module, that module must provide that list.

\$\endgroup\$
2
votes
\$\begingroup\$

Naming:

  • classes: NameOfClass()
  • constants: NAME_OF_CONSTANT
  • all else: name_of_function(), name_of_variable
  • general: avoid abbreviations in names (e.g. cnt should be counter)
\$\endgroup\$
2
votes
\$\begingroup\$

Consider this:

if (check):
    do_something()

This should be written as:

if check:
    do_something()

That is, you don't need to put stuff in brackets.

\$\endgroup\$
0
votes
\$\begingroup\$

Looking through my own reviews, I'd say that the two issues that come up almost every time are:

  1. Docstrings. It feels as if nearly every review I have written has begun by noting the absence of user documentation in the poster's code. (For example.) As a user I expect to be able to type help(module) or help(module.function) into the Python interpreter, and as a programmer I find that the discipline of specifying exactly what each function and class should do helps enormously to come up with good designs, good code structure, and good names.

  2. Doctests. For some types of code these are a really simple way to combine example code with small test cases. Of course no-one should be fooled into thinking that they are a substitute for a proper test suite, but they are so easy to write that they deserve to be much more widely known and used. For example, here's a question containing a bug that would have been easily caught by a simple doctest or two. And here's another.

The third most common problem is probably inappropriate choice of data structures and algorithms (for example, trying to write a parser without a lexer). But that's not really specific to Python.

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged .