The code review that never happened

    AKA try/except properly and log!
    03 November 2017 1863 words

    I am currently working as consultant for a mid-sized company in Europe, where my goal is to try to keep their old Django application alive (running Django 1.5.5).

    A few weeks ago, it became clear to me that the server running the application was down.

    Something wasn't working properly, and yet the logs were completely silent.

    So I said… well, there must be an except: pass somewhere.

    So I started looking for it (so much for my Friday night…)

    screaming code

    This code was running somewhere in production. So I just removed the comments and the code inside each block - everything else I left just as it was.

    So, let's imagine for a second that we went back 4 years (November 2013), and that my colleague, named Charlie (random name), had just made a PR to add this code to our codebase.

    Let's begin.

    Hey Charlie, thanks for the PR.

    The code logic looks ok, but there are a few things that I would definitely change.

    1. Did you forget the docstring?

    Charlie have you read this?

    I think it's always good practice to add docstrings: to at least try to explain, even just briefly, how your function behaves, and maybe also add something about its parameters. It could be invaluable for your colleagues or, indeed, anyone who is going to be working on the code in the future (.......).

    The name of the function really does not tell us that much about the ‘story’ of your code. A general explanation about how your are dehydrating your data would be very much appreciated!

    2. Don't except: pass

    Charlie, I see from the PR that you worked on this very late in the evening..... perhaps this explains it.

    Nonetheless, what were you trying to do with except: pass?

    You are silently catching all the errors, without even logging them.

    In [2]:
    try:
        1/0
    except:
        pass
    

    Do you see?

    No error is raised, even if we are dividing by zero.

    In [4]:
    try:
        1/0
        CONSTANT = 0
        CONSTANT["DON-T-EXIST"] = 0
    except:
        pass
    

    Same goes here - you are silently catching every kind of exception.

    So please:

    • remove pass
    • log this exception somehow, the logging module is a good place to start.
    • be more specific about the exception you want to catch (check the example)
    • don't except: pass
    In [6]:
    # example for Charlie
    
    import logging
    
    log = logging.getLogger(__name__)
    
    try:
        1/0
    except ZeroDivisionError:
        log.error("Division by Zero")
    
    Division by Zero
    

    In any case - great job. Keep up the hard work.

    And, just in case you have forgotten: - please don't except: pass.

    Sincerely, your colleague and a future reader of your code.

    Takeaways for the current readers

    1. Code Reviews

    Do code reviews: - they are extremely useful, not only to pick up on bad practice.

    2. Docstrings

    Use docstrings: - they will be extremely useful in the future,

    3. Catch errors

    Try/Except is really powerful, but use it properly: - to catch specific errors and log them.

    4. Feedback

    Proper feedback is fundamental if you want to grow, both in technical skills and in what are generally called soft skills.

    I say proper feedback, because feedback given in the wrong way is probably more harmful than not giving a feedback at all.

    If you want to know more about how to give proper feedback, here’s a good starting point: http://www.ustream.tv/recorded/102892183.