[Tutor] code review please

[Danny Yoo]

Hi William,

Here are some constructive comments on the program; if you have questions
on any of it, please feel free to ask.


> def fileScramble(fileName):

We may want to rename "fileName" to something else, as it isn't being
treated as a filename, but more like a file-like object.  Good names help
to clarify programs.  I had expected the input here to be a string, and to
see it be a file-like object was a little disorienting at first.


[within fileScramble() ...]

>     newFile = file('scrambled.txt', 'w')
>     newRow = ''
>     for line in fileName:

It's possible that the lines here still have their trailing newlines: you
might want to rstrip() them out before further processing.



> def wordScramble(word):
>     punctuation = ['.', ',', ':', ';', '(', ')']
>     if len(word) < 4:
>         return word
>     elif len(word) == 4 and word[-1] in punctuation or word[0] in
> punctuation:


The complexity of the elif is a bit high.  Do you mean:

    ((len(word) == 4 and word[-1] in punctuation) or
     word[0] in punctuation)

or do you mean:

    len(word) == 4 and (word[-1] in punctuation or
                        word[0] in punctuation)

Explicit parenthesization is a good idea here, since there are two
possible ways of parsing the expression.  It's unambiguous to the computer
of course, but since it's ambiguous for humans, let's make it explicit
what we mean.


> def getLetter(string, number, direction):
>     if direction == 'forward':
>         increment = 1
>     else:
>         increment = -1
>     if string[number].isalpha() == True:


The above if statement can be simplified to:

    if string[number].isalpha(): ...

because comparing True to True is a bit redundant.


In fact, since either string[number].isalpha() is True or it isn't, this
allows us simplify the block:

    if string[number].isalpha() == True:
        ...
    elif string[number].isalpha() == False:
        ...

so that we use a simpler if/else:

    if string[number].isalpha():
        ...
    else:
        ...



Looking into the __main__, I see:

>             try:
>                 f = open(sys.argv[1])
>                 fileScramble(f)
>                 f.close()
>             except IOError:
>                 print "That file does not exist, or you do not have

In the exception handler, we may want to also show the message of the
exception too, just in case another kind of IOError has occurred.


According to:

    http://www.python.org/doc/lib/module-exceptions.html

we can grab at the 'errno' and 'strerror' attributes of the exception
object to display more detailed information --- I'd recommend including
those in the error output, since that's useful debugging information.

Similarly, the try/except for IndexError seems too pervasive: rather than
wrap it around the whole program, we may want to limit its extent to just
around the sys.argv access.  Otherwise, any other IndexError has the
potential of being misattributed.  Much of the program does indexing of
some sort, so that's why this concerns me.

Alternatively, doing the explicit length check:

   if not sys.argv[1:]:
       print some kind of usage message
       raise SystemExit

might be sufficient.


Unless we really want to hide errors from the user, I'd avoid the
exception handlers altogether: if bad things happen, the default exception
handler gives a fairly good stack trace that aids debugging.

But if we do want to handle the exceptions manually, we should try to make
sure that useful information is preserved in the error messages: it's a
terrible thing to get an error message that says "Something bad happened."
when we can do much better.  *grin*


I hope this critique helps!