Ron Phillips <RPhillips <at> engineer.co.summit.oh.us> writes:
> The script produces expected results, but if anyone is so inclined, I'd
appreciate review/evaluation/critique of the 'pythonism' of the script.
I haven't read it from A to Z, but it looks OK. Some minor things:
- string formatting, e.g.:
out+= 'Attribute: '+field+" => "+str(self.fileAttributes[field])+'\n'
I'd prefer seeing a format string. "Attribute: %s => %d\n" % (field,
self.fileAttributes[field]) - I'm guessing at types here, with file as string
and attrib as integer.
- lists of strings and ''.join instead of +, e.g.:
I'd favor making 'out' a list of strings and building the result at the end,
using "\n".join(out).
- long functions/loops: there are two parse functions which don't fit on my
screen. Perhaps they can be split in smaller parts. There's some duplicate code
in the two large (and also the third smaller) parse functions, indicating that
splitting in smaller pieces should be possible and would also remove the
duplicate code.
- There's a lot of "struct.unpack('>i',indexfile.read(4))[0]" and similar lines.
A file class which would wrap this stuff and offer a nice interface with methods
like ReadInt, ReadDouble would clear it up a bit.
- "Private function" in the comment of __newdemo. I'd remove it, since the
double underscore already documents this.
Yours,
Andrei
_______________________________________________
Tutor maillist - Tutor@python.org
http://mail.python.org/mailman/listinfo/tutor
