3
\$\begingroup\$

I need to get this program reviewed. It is counting the lines of code in all files in the given directory.

I'm new to python and need some advice on pretty much everything.

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

def CountFile(f):
    counter = 0
    f = open(f, "r")
    for line in f.read().split('\n'):
        counter = counter + 1
    f.close()
    return counter

def CountDir(dirname):
    counter = 0
    for f in os.listdir(dirname):
        fa = os.path.join(dirname, f)
        if os.path.isdir(fa):
            dcount = CountDir(fa)
            counter = counter + dcount
        else:
            fcount = CountFile(fa)
            counter = counter + fcount
    return counter

print CountDir(sys.argv[1])
\$\endgroup\$
  • 2
    \$\begingroup\$ What was wrong with wc -l *? \$\endgroup\$ – Gareth Rees Feb 2 '14 at 21:15
3
\$\begingroup\$

First a small nitpick: in python the prevailing naming convention for function names is all lowercase, with words separated by underscores. So CountFile should be count_file, or even better would be something like count_lines, since count_file doesn't really explain what the function does.

Now in CountFile, you read the file using f.read() then split on newlines. This isn't really necessary -- for one thing because file objects have a method called readlines which returns a list of the lines, but more importantly because file objects are iterable, and iterating over them is equivalent to iterating over the lines. So for line in f.read().split('\n'): is equivalent to for line in f:, but the latter is better because when you call read() you actually read the entire contents of the file into memory, whereas for line in f: reads the lines one at a time. It's also the idiomatic way to go about reading a file line by line.

If you're not concerned about memory, however, then because flat is better than nested you can do away with the explicit for loop and just call len on your list of lines. The entire function could be return len(open(f, 'r').readlines()) (but this isn't really advisable in the general case because of the memory thing). You could also use a generator expression and write return sum(1 for line in f).

Now notice that in CountFile, you write f = open(f, "r") to open a file, then later you write f.close() -- this kind of pattern is what context managers and the with statement were introduced for. The context manager protocol is already conveniently implemented for file objects, so you could write:

counter = 0
with open(f, 'r') as f:
   for line in f:
       counter += 1

And you wouldn't need to explicitly close the file. This may not seem like such a big deal in this case, and it isn't really, but with statements are a convenient way to ensure you that you only use resources (like files, or database connections, or anything like that) just as long as you need to. (Also, allocating and later freeing a resource might be a lot more involved than just calling open and close on it, and you might have to worry about what happens if something goes wrong while you're using the resource (using try... finally)... Hiding that behind a with statement makes for much more readable code because you can focus on what you're trying to accomplish instead of details like that.)

Your CountDir function looks mostly okay. You might look into using os.walk instead of os.listdir so you won't need to check for directories -- it would look something like

for root, _, files in os.walk(dirname):
    for f in files:
        count += CountFile(os.path.join(root, f))

(The _ doesn't have any special meaning, it just by convention means that we won't be using that variable.) Another advantage of os.walk is that by default it doesn't follow symlinks, whereas listdir doesn't care, so if you don't explicitly check for symlinks you might end up counting things twice.

If you stick with os.listdir, you could still make your code a bit cleaner by using a conditional expression, which would look like:

counter += CountDir(fa) if os.path.isdir(fa) else CountFile(fa)
\$\endgroup\$
1
\$\begingroup\$

You can iterate over a file object line by line without calling .read() or .split('\n') That should save you a bunch of function calls.

Maybe the built in sum() function would be faster?

counter = sum(1 for line in f)
\$\endgroup\$

Your Answer

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