3
\$\begingroup\$

I'm looking for your opinions on anything that I am doing wrong in the application below, such as best practices, glaringly horrible errors or even just your own personal opinion.

Task: Grab an Nginx access log, ask it some questions and dump responses out to a templated report.

I have removed all comments from this post to keep the reading to a minimum. You can see the full version at GitHub.

#!/usr/bin/python
import datetime
from os import remove
from subprocess import Popen
from subprocess import PIPE
from subprocess import check_output
from string import Template

class NginxReporter():

    location = None
    template = 'templates/default.html'
    html = None

    def __init__(self, location=None, template=None, execute=False):
        self.location = location

        if template:
            self.template = template

        if execute:
            self.parse()
            self.output()
            self.delete()

    def scpdownload(self, remotelocation=None):
        if remotelocation:
            check_output(['scp', remotelocation, self.location])

    def delete(self):
        remove(self.location)

    def parse(self):
        html = {}

        codes = Popen("awk '{print $9}' " + self.location +
                       " | sort | uniq -c | sort -r", shell=True, stdout=PIPE)

        html['codes'] = self.build(codes)

        statuses = [206, 301, 302, 400, 404, 408, 499, 500]

        for status in statuses:
            statuslist = Popen("awk '($9 ~ /" + str(status) + "/)' "
                               + self.location + " | awk '{print $7}' " +
                               " | sort | uniq -c | sort -r",
                               shell=True, stdout=PIPE)

            html["http_"+str(status)+"_code"] = self.build(statuslist)

        html['generated'] = datetime.datetime.today().strftime('%Y-%m-%d %H:%M')

        self.html = html

    def build(self, output):
        block = ""

        while True:
            out = output.stdout.read()

            if out == '' and output.poll() != None:
                break
            if out != '':
                block = out

        return block

    def output(self):
        file = open(self.template)
        block = Template(file.read())
        print block.substitute(self.html)

Usage

report = NginxReporter('access.log')
""" report.scpdownload("sshuser@server.com:/path/to/nginx/access.log") """
report.parse()
report.output()
\$\endgroup\$
3
\$\begingroup\$

The biggest problem, and the hardest to fix, is calling awk, sort | uniq -c | sort -r, sometimes within a loop, when you could implement all that more efficiently in Python. It's ugly to do this in multiple sub-shells and multiple processes. But that would take a substantial rewrite.


You are not using the class attributes here:

class NginxReporter():

    location = None
    template = 'templates/default.html'
    html = None

So you could just delete these from your class.


The constructor is not well designed. You're trying to do too much, for two very different use cases:

  1. In one use case, you use it just to set a logfile location, and then you call other methods to do the real work
  2. In another use case, the constructor would call the other methods

It's better to decide one of these ways, stick with it, and simplify the code accordingly.


This method is strange:

def scpdownload(self, remotelocation=None):
    if remotelocation:
        check_output(['scp', remotelocation, self.location])

You allow calling it without params, or null param, to do nothing? This is messy and confusing. It would be better to require a param.

Btw, downloading a file with scp doesn't sound like it belongs to the class. It violates the single responsibility principle: a class should have one clear purpose (parse nginx log files), and nothing else.


This could use some reworking:

if out == '' and output.poll() != None:
    break
if out != '':
    block = out

Like this:

if not out:
    if not output.poll():
        break
else:
    block = out

You open a file for reading but you're not closing it:

file = open(self.template)
block = Template(file.read())
print block.substitute(self.html)

The right way to do is using with:

with open(self.template) as fh:
    block = Template(fh.read())
    print block.substitute(self.html)

I also renamed file to fh because file is a class in Python 2.x


If you want to temporarily comment out one line, it's better to use # instead of """ ... """ which is recommended for multi-line comments and docstrings:

report = NginxReporter('access.log')
#report.scpdownload("sshuser@server.com:/path/to/nginx/access.log")
report.parse()
report.output()
\$\endgroup\$
  • 1
    \$\begingroup\$ Fantastic! I mean really great feedback. I will modify code appropriately and update for anyone else who stumbles across this to chip in. \$\endgroup\$ – Neil Masters Sep 22 '14 at 7:45
  • \$\begingroup\$ Glad you like it! For the record, you're not supposed to edit the code in your post as that would invalidate my answer ;-) \$\endgroup\$ – janos Sep 22 '14 at 7:57
  • \$\begingroup\$ Updated code example. I am still confused by your point that the class variables are not being used. I have updated the code(tbh it did not even need a constructor with my default values) so perhaps that changes something. \$\endgroup\$ – Neil Masters Sep 22 '14 at 8:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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