10
\$\begingroup\$

I am fairly new to Python programming language. Most of python programmers have heard the philosophy behind the python programming language. The code should be easy to read, easy to understand, simple, fast. I would like to this post to guide users on how to make codes more 'Pythonic' from examples of my script.

I have recently developed a script, to parse Apache log files, and calculate (analyse) it by showing the IP counts, Status counts, amount of bytes transferred. The user can choose the output he wants to get.

As I was reading my code, I realised, that it's very large, and could be optimised. I want to mention that I am not an experienced code writer either, so it obviously does have some flaws in it. Saying that it 'works' so it's 'OK' is kind of interfering with the philosophy itself.

That's where my question derives from: How do I make it more 'Pythonic', more simple, more easy to read?

#!/usr/bin/env python
import os
import sys

if __name__ == '__main__':
    filename = sys.argv[1]

    try:
        with open(filename, 'r') as logfile:
            ip = []
            bytes = []
            status = []
            for line in logfile:
                split = line.split()
                ip.append(split[0])
                bytes.append(split[9])
                status.append(split[8])
    except OSError:
        print(filename, 'not existing')
        exit()
    except IndexError:
        print(filename, 'format not in CLF')
        exit()

    ip_list = []
    status_list = []

    ip_count = []
    status_count = []

    sort = int(
        input('Do you want to sort results by ip[1] or status[2]? [ANSWER]: '))
    if sort == 1:
        for match in (ip):
            if match not in ip_list:
                ip_list.append(match)
    if sort == 2:
        for match in (status):
            if match not in status_list:
                status_list.append(match)

    desired_output = int(input(
        'Choose Desired Output: [count] -o- [percentage] -o- [bytes]: '))
    if sort == 1:
        for match in ip_list:
            count = 0
            for ip_match in ip:
                if match == ip_match:
                    count += 1
                ip_count.append(count)

        if desired_output == 1:
            ip_count, ip_list = zip(
                *sorted(zip(ip_count, ip_list), reverse=True))
            for i in range(len(ip_list)):
                print('IP: ' + str(ip_list[i]) + ' count: ' + str(ip_count[i]))
        if desired_output == 2:
            ip_count, ip_list = zip(
                *sorted(zip(ip_count, ip_list), reverse=True))
            for i in range(len(ip_list)):
                print('IP: ' + str(ip_list[i]) + ' percentage: ' +
                      str(round(ip_count[i]/len(ip)*100, 2)) + '%')
        if desired_output == 3:
            cnt_bytes = []
            for v in range(len(ip_list)):
                tmp = 0
                for k in range(len(ip)):
                    if ip_list[v] == ip[k]:
                        if bytes[k] == '-':
                            bytes[k] = 0
                        tmp += int(bytes[k])
                    cnt_bytes.append(tmp)
                ip_list, cnt_bytes = zip(
                    *sorted(zip(cnt_bytes, ip_list), reverse=True))
                for line in range(len(ip_list)):
                    print('IP: ' + str(ip_list[line]) +
                          'bytes: ' + str(cnt_bytes[line]))
    
    if sort == 2:
        for match in status_list:
            count = 0
            for status_match in status:
                if match == status_match:
                    count += 1
                status_count.append(count)

        if desired_output == 1:
            status_count, status_list = zip(
                *sorted(zip(status_count, status_list), reverse=True))
            for i in range(len(status_list)):
                print('Status: ' + str(status_list[i]) + ' count: ' + str(status_count[i]))
        if desired_output == 2:
            status_count, status_list = zip(
                *sorted(zip(status_count, status_list), reverse=True))
            for i in range(len(status_list)):
                print('Status: ' + str(status_list[i]) + ' percentage: ' +
                      str(round(status_count[i]/len(status)*100, 2)) + '%')
        if desired_output == 3:
            cnt_bytes = []
            for v in range(len(status_list)):
                tmp = 0
                for k in range(len(status)):
                    if status_list[v] == status[k]:
                        if bytes[k] == '-':
                            bytes[k] = 0
                        tmp += int(bytes[k])
                    cnt_bytes.append(tmp)
                cnt_bytes, status_list = zip(
                    *sorted(zip(cnt_bytes, status_list), reverse=True))
                for line in range(len(status_list)):
                    print('Status: ' + str(status_list[line]) +
                          'bytes: ' + str(cnt_bytes[line]))

Example log file:

74.125.176.81 - - [17/May/2015:11:05:28 +0000] "GET /?flav=rss20 HTTP/1.1" 200 29941 "-" "FeedBurner/1.0 (http://www.FeedBurner.com)"
66.249.73.135 - - [17/May/2015:11:05:14 +0000] "GET /blog/geekery/xdotool-2.20110530.html HTTP/1.1" 200 11936 "-" "Mozilla/5.0 (iPhone; CPU iPhone OS 6_0 like Mac OS X) AppleWebKit/536.26 (KHTML, like Gecko) Version/6.0 Mobile/10A5376e Safari/8536.25 (compatible; Googlebot/2.1; +http://www.google.com/bot.html)"
187.45.193.158 - - [17/May/2015:11:05:54 +0000] "GET /presentations/logstash-1/file/about-me/tequila-face.jpg HTTP/1.1" 200 196054 "-" "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.2; SV1; .NET CLR 2.0.50727; InfoPath.1)"
\$\endgroup\$
0

1 Answer 1

16
\$\begingroup\$

If code is repetitive, you probably need some functions and better data organization. The current code is very repetitive: multiple versions of nearly-but-not-quite identical code to handle the different ways of sorting and aggregating the information. The solution is such cases usually involves some combination of (1) figuring out a way to generalize the needed behavior in one or more functions; and (2) organizing the program's information more effectively into collections of meaningful objects.

Parse log lines into meaningful objects, not into three parallel lists. Currently, you extract three pieces of information from each line (IP address, N of bytes, and HTTP status code) and store that information in separate lists. This is a fateful step because it pushes you in a direction where each user choice about sorting and aggregating requires slightly different code. A more flexible approach is to convert the raw data (a collection of log lines) into meaningful data (a collection of objects holding the facts you care about from each line). Here's one way to approach it. The mindset here is to focus each function or class on a very small part of the problem: LogEntry to hold facts about one log line; main to deal with user inputs/outputs and overall program orchestration; parse_log_file to read the log file; and parse_log_line to convert a single line to a LogEntry.

import sys
from dataclasses import dataclass

@dataclass(frozen = True)
class LogEntry:
    ip_address : str
    n_bytes : int
    status_code : int

def main(args):
    file_path = args[0]
    entries = parse_log_file(file_path)
    for e in entries:
        print(e)

def parse_log_file(file_path):
    try:
        with open(file_path) as log_file:
            return [parse_log_line(line) for line in log_file]
    except OSError:
        abort(f'File not found: {file_path}')

def parse_log_line(line):
    try:
        xs = line.split()
        return LogEntry(xs[0], int(xs[9]), int(xs[8]))
    except IndexError:
        abort(f'Invalid log file format: {file_path}')

def abort(msg):
    print(msg, file = sys.stderr)
    exit(1)

if __name__ == '__main__':
    main(sys.argv[1:])

If you need to count things, consider using a Counter. Your current approach to counting things is extremely complex -- to such a degree that I lacked the patience to figure out entirely what you are trying to do. To keep things focused here, I'll assume the following simplified scenario: we want to parse the log lines and output either counts or percentages for any of the three LogEntry attributes.

Embrace command-line arguments fully. You're already taking the file path on the command line. Do the same for sorting and aggregation method. Get those values from the command line, typically by using the argparse library. It's the most common strategy for scripts like this. Our argument parser needs a file path, the LogEntry field to be aggregated, and a flag to request percentages rather than counts. Here's how we could do that:

import argparse

def main(args):
    opts = parse_args(args)
    entries = parse_log_file(opts.file_path)
    n_entries = len(entries)
    tally = Counter(getattr(e, opts.field) for e in entries)
    for k, n in tally.items():
        pct = n / n_entries
        print(k, pct if opts.percent else n)

def parse_args(args):
    # Command-line usage: FILE_PATH FIELD [--percent]
    ap = argparse.ArgumentParser()
    ap.add_argument('file_path')
    ap.add_argument('field', choices = ('ip_address', 'n_bytes', 'status_code'))
    ap.add_argument('--percent', action = 'store_true')
    return ap.parse_args(args)

The code shown above does not do exactly what you need. But it does provide a better foundation upon which you could build this kind of log aggregating script.

\$\endgroup\$
2
  • \$\begingroup\$ The code shown above does not do exactly what you need. => Which I would argue is good, because SO is not a code-writing service ;) \$\endgroup\$ Commented Feb 28, 2022 at 16:05
  • \$\begingroup\$ And nor is CR! ;) \$\endgroup\$ Commented Mar 1, 2022 at 9:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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