Re: Evaluate coding and style
Rhodri James wrote: On Thu, 24 Sep 2009 21:11:36 +0100, Brown, Rodrick rodrick.br...@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) [snip] It's is usually a bad idea to loose information when giving feedback to the user. try: fh = open(filename) except IOError, exception: print Error openning %s : % (filename, exception) In general, if you want to improve your coding style/rules, read PEP 8 and this page : http://tottinge.blogsome.com/meaningfulnames/ This is a must read (yet you still can disagree). Regarding this, fh = open(filename) would become fileHandler = open(fileName) Much nicer to read :o) These are just suggestions, cheers. JM -- http://mail.python.org/mailman/listinfo/python-list
Re: Evaluate coding and style
Brown, Rodrick 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.
All in all it looks pretty good. Some minor enhancements...
$ ./homedir_exists.py root mqm pcap
root successful!
Directory: /var/arpwatch not found!
false negative?
pcap successful!
mqm successful!
$ cat homedir_exists.py
#!/usr/bin/env python
import sys, os
from re import match
userlist = []
filename = '/etc/passwd'
for user in sys.argv[1:]:
userlist.append(user)
since sys.argv is already a list, you can do
userlist = sys.argv[1:]
try:
fh = open(filename)
except IOError:
print No such filename: %s % (filename)
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!
checkDir(userlist)
passwd has a well defined layout... instead of using re, I would split
each line into it's component fields, then just match the username
fields against the usernames passed in... this also avoids the false
negative shown in your example, and gets rid of one unnecessary loop.
for line in data:
fields = line.split(':')
user = fields[0]
path = fields[5]
if user in userlist:
if os.path.isdir(path):
print %s successfull! % user
else:
print Directory %s for user %s not found! % (path, user)
Cheers!
~Ethan~
--
http://mail.python.org/mailman/listinfo/python-list
Re: Evaluate coding and style
On 24 Sep, 21:11, Brown, Rodrick rodrick.br...@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
--
http://mail.python.org/mailman/listinfo/python-list
Re: Evaluate coding and style
On Thu, 24 Sep 2009 21:11:36 +0100, Brown, Rodrick
rodrick.br...@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
--
http://mail.python.org/mailman/listinfo/python-list
Re: Evaluate coding and style
Brown, Rodrick 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. You may want to check out pylint ( pychecker) Esmail -- http://mail.python.org/mailman/listinfo/python-list
