6
\$\begingroup\$

Pardon if this is silly, but I'm pretty new to coding. I have this code, that works, but I feel like it could be tighter than this, I just can't seem to get it to work any other way. The idea is to read a .txt file find incoming e-mail strings and organize the data by frequency of hour sent.

Here is an example line that I'm looking for in the text:

From email@emailaddress.com Sat Jan 5 09:14:16 2008

Here is my code:

fname = input("Enter file:")
if len(fname) <1 : fname = "filename.txt"
fh = open(fname)
time = list()
hours = list()
hr = dict()

for line in fh:
        if not line.startswith("From "): continue
        words = line.split()
        time.append(words[5])

for i in time:
        i.split(":")
        hours.append(i[:2])

for h in hours:
        hr[h]=hr.get(h, 0)+1

l = list()
for k,v in hr.items():
        l.append((k,v))
l.sort()
for k, v in l:
        print (k,v)
\$\endgroup\$
0
7
\$\begingroup\$

Formatting

Please follow PEP8, the Python style guide. Most noticeably, use 4 spaces to indent.

Working with files

The recommended way to work with files is using with ... as, like this:

with open(fname) as fh:
    for line in fh:
        if not line.startswith("From "):
            continue
        words = line.split()
        time.append(words[5])

This way, you don't have to remember to close the filehandle when you're done using it.

Naming

Use plural names for collections. time is really odd for a list of time strings. And I suppose that's part of the reason for naming the loop variable i here:

for i in time:
    i.split(":")
    hours.append(i[:2])

When it would have been so much more natural as:

for time in times:
    time.split(":")
    hours.append(time[:2])

Pointless statements

i.split here is completely pointless. It splits the string, but then the result is never used:

for i in time:
    i.split(":")
    hours.append(i[:2])

This is the same thing:

for i in time:
    hours.append(i[:2])

Use list comprehensions

The hours list can be created simply as:

hours = [time[:2] for time in times]

Use collections.Counter

Instead of this:

for h in hours:
        hr[h]=hr.get(h, 0)+1

You could create hr a lot simpler:

from collections import Counter

hr = Counter(hours)

(Of course, the import statement should be at the top of the script.)

Sorting

Instead of this:

l = list()
for k, v in hr.items():
    l.append((k, v))
l.sort()
for k, v in l:
    print(k, v)

You could use the builtin sorted, and greatly simplify:

for hour, count in sorted(hr.items()):
    print(hour, count)

Notice that I renamed the loop variables. How much easier to read that!

Use functions

Rather than just a sequence of statements, it would be better to decompose your logic to functions.

Suggested implementation

With the above suggestions applied (+ PEP8, do read that!), and then some more:

from collections import Counter

DEFAULT_FILENAME = 'filename.txt'


def input_filename():
    filename = input("Enter file:")
    if not filename:
        filename = DEFAULT_FILENAME
    return filename


def read_times(filename):
    with open(filename) as fh:
        for line in fh:
            if line.startswith("From "):
                yield line.split()[5][:2]


def main():
    filename = input_filename()

    hour_counts = Counter(read_times(filename))

    for hour, count in sorted(hour_counts.items()):
        print(hour, count)

if __name__ == '__main__':
    main()
\$\endgroup\$
0
3
\$\begingroup\$

I think that this task is trivial enough in Python that I would aim for a short-and-sweet solution:

from collections import Counter 
import fileinput

from_lines = (line for line in fileinput.input() if line.startswith('From '))
hours = (line.split()[5].split(':')[0] for line in from_lines)
for hour, count in sorted(Counter(hours).items()):
    print(hour, count)

In particular,

  • Instead of prompting for a filename, use fileinput. Not only does it simplify your code, it also lets the program accept multiple filenames on the command line to be analyzed at once, or even input piped from the network. fileinput just makes your program work better as a command-line tool.
  • Prefer generator expressions over .append() loops, since you don't actually need to keep the entire list.
  • Similarly, use sorted() if you don't need to keep a sorted list.
  • Use Counter instead of a manual loop to count items.
  • Avoid meaningless variable names that lead to cryptic expressions like for k, v in l: ….

Note that line.split()[5].split(':')[0] is a bit nasty to follow. Instead of taking everything before the first colon of the sixth word, you could use a regular expression to look for a string that looks like "HH:MM:SS". That would be more readable, in my opinion.

from collections import Counter
import fileinput
from re import search

from_lines = (line for line in fileinput.input() if line.startswith('From '))
hours = (search(r'(\d{2}):\d{2}:\d{2}', line).group(1) for line in from_lines)
for hour, count in sorted(Counter(hours).items()):
    print(hour, count)
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Using a regex will also work with from lines like From "My name" <email@emailaddress.com> Sat Jan 5 09:14:16 2008, which is allowed in some mailbox formats... \$\endgroup\$ – holroy Nov 20 '15 at 22:39

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.