2
\$\begingroup\$

Currently I'm trying to redo this code to avoid the breaking line because of the 80 characters rule:

def decompress_all(directory_path, remove_after):
    """
    """
    try:
        if 0 in [ decompress(file_path, remove_after) \
                  for file_path in files_within(directory_path) ]:
            decompress_all(directory_path, remove_after)
        return 0
    except:
        return 1

Python version 2.6.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Beyond PEP8 \$\endgroup\$ Commented Jan 12, 2017 at 14:06
  • \$\begingroup\$ @MathiasEttinger I wish this video were linked more often in Python questions. \$\endgroup\$
    – mkrieger1
    Commented Jan 12, 2017 at 22:12

2 Answers 2

6
\$\begingroup\$
  1. The docstring is empty. What is this function supposed to do? What does it return?

  2. A bare except: is a bad idea, because it catches all exceptions, including KeyboardInterrupt and SystemExit. If you have some particular exception that you want to catch, then name it. But do you really need to catch any exceptions here? By catching exceptions you conceal information about what happened — instead of an error message and a traceback, all you get is the number 1, which tells you nothing.

  3. The result of the recursive call to decompress_all is discarded and so is lost. If there were an exception inside the recursive call you'd never find out about it.

  4. In POSIX, it's conventional for functions to return 0 for success and non-zero for failure. But in Python, it's conventional for functions either to return True for success and False for failure, or to return nothing for success, and raise an exception for failure.

  5. The function calls itself with the same arguments. Why does it do this? What prevents this from recursing forever? I guess the idea here is that multiple layers of decompression might need to be undone, for example if one .zip file contains another .zip file. But I think this is not at all obvious and deserves a comment.

  6. Instead of recursing, use a loop.

Revised code:

def decompress_all(directory_path, remove_after):
    """Repeatedly decompress all files under directory_path until no more
    files can be decompressed. The argument remove_after is passed to
    decompress.

    """
    # Decompression may extract more compressed files, so loop
    # until no more files need to be decompressed.
    while sum(decompress(filename, remove_after) == 0
              for filename in files_within(directory_path)):
        pass
\$\endgroup\$
2
\$\begingroup\$

If you really think that not breaking the 80 chars limit is more important than having compact code, you can try something like this:

def decompress_all(directory_path, remove_after):
    """
    """
    must_decompress = False
    try:
        for file_path in files_within(directory_path):
            if decompress(file_path, remove_after) == 0:
                must_decompress = True

        if (must_decompress):
            decompress_all(directory_path, remove_after)
        return 0
    except:
        return 1
\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.