Evaluate coding and style

[Rhodri James]

On Thu, 24 Sep 2009 21:11:36 +0100, Brown, Rodrick   
<rodrick.brown at citi.com> wrote:

> I recently started playing with Python about 3 days now (Ex Perl guy)  
> and wanted some input on style and structure of what I'm doing before I  
> really start picking up some bad habits here is a simple test tool I  
> wrote to validate home dirs on my system.

Ethan's made some good points.  Here are a couple more.

[snip]
> try:
>   fh = open(filename)
> except IOError:
>   print "No such filename: %s" % (filename)

IOError could be raised for a couple of reasons, not just because the
file doesn't exist, so your printed message is a bit misleading.  Worse,
because you've caught the exception you'll carry on executing, but
with no open file!  checkDir will then complain, and it will all fall
over in one big, misleading mess.

On the whole, it's probably better not bothering with the try...except
since the default behaviour is likely to be what you want anyway.

> def checkDir(username):

PEP 8 (the style guide) suggests that function names should be
lowercase_with_underscores.  It's only a suggestion, mind you.

>   data = fh.readlines()

Tsk.  Using the fh as a global variable, rather than passing it as
a parameter?  Slapped wrists all round!

In fact the division between what's in the function and what's not
is rather arbitrary at the moment.  I'd be tempted to move the file
opening into the function it would make it a little easier to convert
this script into a module if you ever wanted to.

>   for line in data:
>     for user in username:
>       if match(user,line):
>         s = line.split(':')
>         if not os.path.isdir(s[5]):
>           print "Directory: %s not found!" % (s[5])
>         print s[0] + " successful!"

Apart from Ethan's fix, there's another issue here.  You don't
catch the case of a username given on the command line not actually
existing in the password file.  That probably means updating the
username list (which is hard if you're iterating over it at the
time) or creating a list of what's been found as you go, and being
aware of the possibility of duplicates.

> checkDir(userlist)

If you're going to stuff everything into a function and then
execute it (not a bad idea at all), then there's another idiom
you should know about.

if __name__ == '__main__':
   checkDir(userlist)

(though in reality again you'd think about what you wanted in
checkDir and bung everything else inside the 'if')

The special variable __name__ contains the name of the current
module.  For a script (executing directly from the command line),
the module name is always "__main__".  So when you run
homedir_exists.py from the command line, the condition is true
and your script runs exactly as it does now.

If however you tweak it along the lines I've suggested, you can
use it as a module as well:

-=-=-=-
$ cat trivial_example.py
 from homedir_exists import checkDir

# Do something important
checkDir("fred")
# Do something else important
-=-=-=-

In this case, when homedir_exists.py is read in, __name__
will be the name of the module (i.e. "homedir_exists") instead
of "__main__", so the original "checkDir(userlist)" won't be
executed.

-- 
Rhodri James *-* Wildebeest Herder to the Masses