Evaluate coding and style

[Jon Clements]

On 24 Sep, 21:11, "Brown, Rodrick " <rodrick.br... 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.
>
> Please evaluate and let me know what could have been done better. Once again this is really my first time using python.
>
> $ ./homedir_exists.py root mqm pcap
> root successful!
> Directory: /var/arpwatch not found!
> pcap successful!
> mqm successful!
>
> $ cat homedir_exists.py
> #!/usr/bin/env python
>
> import sys, os
> from re import match

Imports are typically one per line. Personally I'd just use import re,
as re.match isn't too much typing and makes it fairly clear to any
Python programmer it's a regex match (as opposed to a filename match
or wildcard match or other such thing)

>
> userlist = []
> filename = '/etc/passwd'

I'd probably have filename as FILENAME as it's almost a constant.

>
> for user in sys.argv[1:]:
>   userlist.append(user)

You don't really need to build userlist like this, try:
userlist = sys.argv[1:]



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

If the file doesn't exist then print it doesn't exist and carry on
regardless anyway? Either re-raise another exception, exit the
program, or don't bother with the try/except and just open the file.
If it doesn't exist, the exception will go unhandled and the program
will stop with the IOError exception.


> def checkDir(username):
>   data = fh.readlines()
>   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!"
>

Conventionally the function would be called checkdir or check_dir. I'd
also pass the file object and userlist as parameters instead of
referring to an outer scope.

You don't need the .readlines, just iterate over the fh object as 'for
line in fh'.

'match' is completely the wrong function to use here. A user of 'r'
will match root, roger, etc... etc... Also, as python supports the in
operator, looping each user is unnecessary, something like:

for line in fh:
    tokens = line.split(':')
    if tokens[0] in userlist:
        if not os.path.isdir(tokens[5]):
            print 'Directory %s not found' % tokens[5]
        else:
            print 'User %s successful' % tokens[0]

> checkDir(userlist)

It's fairly traditional to wrap the 'main' function inside a block to
check if this script is the main one that's executing as such:

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

This enables the program to be used in an import so that other
programs can use its functions, but will only execute the check
function, if it's the sole script.


hth a bit,
Jon