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)
wc -l *
? \$\endgroup\$