Email log parser

Question

This code parses mail server log files and outputs statistics into an external file. Is there a way to make it more pythonic? Is it a good idea to use regex anywhere? Which libraries can reduce the length of code?

import sys


deferred_as_error = '--deferred-as-error' in sys.argv

# {session id: email}
logged_in = dict()
# {email: [success:fail]}
result = dict()

with open('maillog', 'r') as log_file:
    for log in log_file:
        if 'sasl_method=LOGIN' in log:
            logged_in[log.split(': ')[1]] = log.split('sasl_username=')[-1][:-1]
        elif 'removed' in log:
            if log.split(': ')[1] in logged_in:
                del logged_in[log.split(': ')[1]]
        elif 'to=<' in log:
            session_id = log.split(': ')[1]
            if session_id in logged_in:
                sender = logged_in[session_id]
                if sender not in result:
                    result[sender] = [0, 0]
                if 'status=sent' in log:
                    result[sender][0] += 1
                elif 'status=bounced' in log or 'status=expired' in log:
                    result[sender][1] += 1
                elif 'status=deferred' in log and deferred_as_error:
                    result[sender][1] += 1


output = open('output.txt', 'w')

print len(result)

for k, v in result.iteritems():
    output.write("{0}: {1} {2}\n".format(k, v[0], v[1]))

output.close()

Answer 1

Is there a way to make code more pythonic?

It would be more Pythonic to handle the output writing using with, like you did for reading. So instead of this:

output = open('output.txt', 'w')

print len(result)

for k, v in result.iteritems():
    output.write("{0}: {1} {2}\n".format(k, v[0], v[1]))

output.close()

Write like this:

with open('output.txt', 'w') as output:
    for k, v in result.iteritems():
        output.write("{0}: {1} {2}\n".format(k, v[0], v[1]))

This way you don't need to worry about closing the file handle, which also makes the code shorter.

---

Another reason I quoted your original code above, because there is a print len(result) in between opening the file and writing to the file. It would make more sense to do the printing first, for several reasons:

• If you don't need the open file at the time you print, then why open the file first?
• If opening the file first, the print statement won't be reached. Is that really what you wanted?

In short, pay attention to put statements in the right order.

---

Avoid doing things twice, like here:

if log.split(': ')[1] in logged_in:
    del logged_in[log.split(': ')[1]]

This code splits the line twice. Split once before the if.

---

Since the default mode of opening a file is for reading, you could omit the 'r' on this line:

with open('maillog', 'r') as log_file:

---

Is it good idea to use regex anywhere?

It depends largely on what you want to achieve. It seems to me that the output would be more interesting to break down by the count of each status, for example sent=5, bounced=1, expired=2. In that case I would rewrite by extracting the status with a regex like status=(\w+), and use the captured value as the key in the counter. Just a thought, this might not be what you want at all.

Which libraries can reduce length of code?

It wouldn't make your code shorter, in fact it would make it longer, but as @jonrsharpe said, argparse would be great for parsing command line arguments. It would make it easier to add more command line options in the future.

Answer 2

The most obvious thing to comment on is the overall structure. As it stands, everything is just at the top level of the script, which makes it very difficult to reuse components later on. Instead, you should move the code into a function, and call it:

import sys

FILENAME = 'maillog'

def parse_log_file(filename):
    """Docstring to explain what it does."""
    ...

if __name__ == '__main__':
    parse_log_file(FILENAME)

You can (and should!) then easily split it further into separate functions for e.g. parsing the log file and writing the output.

---

Rather than simply:

deferred_as_error = '--deferred-as-error' in sys.argv

you could use argparse to create a command line interface for the script. One benefit of this is that it provides --help and input error handling for you.

---

It's notable that you open one file with a context manager (with) and the other you close manually - it's better to at least be consistent, and generally the context manager is recommended.

---

Rather than:

for k, v in result.iteritems():
    output.write("{0}: {1} {2}\n".format(k, v[0], v[1]))

I would either:

1. Be specific about what I'm expecting v to contain (i.e. adding this, the_other = v); or
2. Write it to handle an arbitrary-length v ("...".format(k, ' '.join(v))).

I would also use key and val or value as variable names, assuming you can't think of anything better; I generally only use single-letter names inside generator expressions (and then sparingly) or when it makes sense in terms of the domain (e.g. x and y for coordinates). Looking at the data that goes into the dictionary, perhaps:

for sender, (sent, not_sent) in result.iteritems():
    output.write("{}: {} {}\n".format(sender, sent, not_sent))

---

Rather than just storing [sent_count, not_send_count], I would be inclined to store {reason: reason_count, ...} (see e.g. collections.Counter) at the parsing stage. This simplifies the parsing code, and makes it more resuable, as it doesn't need to worry about things like --deferred-as-error (which can then be applied at the output/reporting stage instead). You could use regex to extract the reason, if you wanted, for example with r'status=(?P<reason>[a-z]+)'.