Need help optimizing first script

[John J. Lee]

popup391 at yahoo.com (Frederic Lafleche) writes:

> I put this script together mainly from bits and pieces I picked up in
> this newsgroup and one or two books lying around. What it does is map
> a given NT network drive and copy a file to it if some conditions are
> met.  As this is my first script, I'm looking for ways to improve code
> in any possible way, whether it's overall design, style, grammar,
> logic, readability, performance, validation, anything.

Use a docstring, not a comment.  A blank line before the imports is
more readable, IMHO.

> #copyacs.py
> import os, shutil, sys, time, win32file
> from win32wnet import WNetAddConnection2, error

Superfluous comment.  We know where there are functions when we see
'def'.

> # Functions

This can be turned into a docstring, in standard format.

http://www.python.org/peps/pep-0257.html

And I presume you mean yyyy, not yyy.

> # Generate timestamp
> # OUTPUT: string in yyymmddhhmm format
> def determineFN():

Another useless comment.  Note that in recent Pythons,
time.localtime's default is time.time() anyway, so you probably don't
need this at all.

>     # Get current time
>     now = time.time()
>     # Parse elements

You don't need all those brackets (certainly not around the RHS), and
the 'I' convention you seem to be using there meant nothing to me --
it slowed me down rather than helped.

>     (Iyear, Imonth, Iday, Ihour, Iminute) = (time.localtime(now)[:5])

You could do this with map -- drop the line above, and:

year, month, day, hour, minute = map(str, time.localtime(now)[:5])

>     year = str(Iyear)
>     month = str(Imonth)
>     day = str(Iday)
>     hour = str(Ihour)
>     minute = str(Iminute)

But wait a minute, you didn't need to map to str in the first place --
you can do this with the string formatting operator %.

    return ("%.4d"+"%.2d"*4) % time.localtime()[:5]

And get rid of all this:

>     # Padding
>     if len(month) == 1:
>         month = '0' + month
>     if len(day) == 1:
>         day = '0' + day
>     if len(hour) == 1:
>         hour = '0' + hour
>     if len(minute) == 1:
>         minute = '0' + minute
> 
>     return year + month + day + hour + minute

So, now we've got that function and those comments down to:

def determineFN():
    """Return timestamp in yyyymmddhhmm format."""
    return ("%.4d"+"%.2d"*4) % time.localtime()[:5]

Note docstring style: for example, wording, spacing and use of
triple-double quotes (see the PEP for why).


But we can do better.

def timestamp():
    """Return timestamp in yyyymmddhhmm format."""
    time.strftime("%Y%m%d%H%M")


You can ditch the function and the docstring altogether, now.  :-)



> # Map network drive
> def use (drive, host, user, passwd):
>     try:
>         WNetAddConnection2 (1, drive, host, None, user, passwd)
>         return 0
>     except error:
>         return 1

Apart from the comment where there should be a docstring, OK.  It's
good to stick to a consistent code formatting style, though, so get
rid of that space between 'use' and the parameter list.

If you're using Python 2.2 or 2.3, you can use True and False there in
place of 1 and 0.  If not, personally I like to introduce something
similar for clarity (at the top of a script or module):

try: True
except NameError:
    True = 1
    False = 0

I'm not 100% sure the function earns its keep, mind you.


What's this comment here for?  Cut it.

> # Variable assignments

A useful, widely-followed and -known convention is to use ALL_CAPS for
globals like these.

As I read your code, I have to guess at this point that you want me
(your user ATM -- but remember 'your user' will be YOU in the future,
when you've forgotten the details of your own code) to fill in the
first five assignments here, and leave the rest to your code to
calculate.  Why should I have to *guess* that?  Split those
assignments off from the rest visually with a blank line or two.  Then
you can get rid of all those repeated 'Enter xxx' bits in the
comments, too, in favour of a single comment explaining that you want
me to fill these in.  Why not then put them at the top of your source
code, where somebody can actually see them, rather than having to read
*all* of your source to figure out how to use it?

I like to use the coding style (more or less) described by Guido van
Rossum (it's a PEP, can't be bothered to look it up for you -- Google
for it).  Some of the things it suggests are tiny details, like using
two spaces before an inline comment.  Trivial, yes, but why not be
standard?  It also says:

foo = 1                           # don't
really_long_variable_name = None  # line up comments like this

These are actually reasonable rules -- slightly more space makes for
readability, and lining up comments uses your time for no good
payback.

> devicename = 'I:' # Enter network drive number.  Ex. format: 'Z:'

Ugh, code mixed up with data I'm supposed to enter.  Use os.path.join
here, not +.  Portability isn't an issue with this script, but it's
clearer, and you'll make fewer mistakes with it.

> dirTarget = devicename + '\\it\\popup' # Enter target path.
> sharename = '\\\\afs\\grps'            # Enter shared resource name.  
> username = 'login' # Enter user account inside single quotes
> password = 'password' # Enter user password inside single quotes
> drives = []                                 
> drivemask = win32file.GetLogicalDrives()    # Bitmask

More pointless comments.  I suppose maybe I'm a bit harsh here: when
you're learning something new, it can be useful to have comments like
this just to remind *yourself*.  As long as you stop doing it pretty
quickly!

> acsMode = sys.argv[1]    # Get second command-line argument of list.
>                          # (First argument is program name.)
> dirSource = 'd:\\oracle\\acs\\' + acsMode   # Source directory
> acsfile = 'import.txt'                      # File to copy
> sourceacsfile = dirSource + '\\' + acsfile  # Absolute path

And there was me thinking the main body already started.  Comment out
of sync with code.  If you get rid of the comment, that can't happen.
Use def main(): instead.  OK, so it's a tiny script, so you could not
bother with a main(), but an explicit function is better than a
comment.

> # Main body
> 
> fDate = determineFN()
> execlog = dirSource + '\\logs\\trace' + fDate + '.log'                

Why is this "Operations log" comment floating down here?  The
universal convention is that comments refer to stuff on the
*following* lines, not preceding lines.

>          # Operations log
> timestring = time.strftime("%Y-%m-%d %H:%M:%S",

Don't need time.time() for recent Pythons.  Oh, wait a minute -- why
is the indentation the same as the previous line?  Very hard to read,
mostly because nobody expects that.  Get a good editor, it'll do that
automatically, and also make some otherwise hard-to-spot mistakes
stand out by refusing to indent your code properly.

> time.localtime(time.time()))    # Timestamp

Another pointless comment.

Hey, wait a minute: I just realised you already knew about strftime!
So why a separate function, AND this use of strftime directly?  At
this point I run out of energy.  :-)


John