[Tutor] code review please

[Kent Johnson]

Eakin, W wrote:
> The code follows, so any comments and/or suggestions as to what I did 
> right or wrong, or what could be done better will be appreciated.

> def fileScramble(fileName):
>     newFile = file('scrambled.txt', 'w')
>     newRow = ''
>     for line in fileName:
>         newRow = ''
>         tempList = line.split(' ')
>         for word in tempList:
>             newRow = newRow + ' ' + wordScramble(word)
>         newRow = newRow + '\n'
>         newFile.write(newRow)

This seem pretty verbose to me. Using tempList doesn't IMO add anything, 
it might as well be
   for word in line.split(' '):
or
   for word in line.split():
since you probably want to split on tabs also and this will strip the 
trailing newlines as a bonus.

I usually prefer a list comprehension to an accumulation loop when 
possible so I would actually write it as
   newWords = [ wordScramble(word) for word in line.split() ]
   newRow = ' '.join(newWords) + '\n'

or even
   newRow = ' '.join(wordScramble(word) for word in line.split()) + '\n'

though that might be a little too terse for easy comprehension.

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

Rather than repeating the test for len(word) == 4, I would write

     elif len(word) == 4:
       if word[-1] in punctuation or word[0] in punctuation:
         return word
       else:
         word = word[0] + word[2] + word[1] + word[3]
         return word

This also breaks up the long conditional that Danny complained about.

>     else:
>         (fCut, fLetter) = getLetter(word, 0, 'forward')
>         (lCut, lLetter) = getLetter(word, -1, 'back')
>         tempWord = list(word)
>         del tempWord[:fCut + 1]
>         del tempWord[lCut:]

I think this is the same as
   tempWord = list(word[fCut+1:lCut])

>         random.shuffle(tempWord)
>         middleString = "".join(tempWord)
>         scrambledWord = fLetter + middleString + lLetter
>         return scrambledWord
> 
> 
> def getLetter(string, number, direction):

I found it very hard to understand what this function does. A better 
name and a comment would help a lot. You might consider having separate 
getFirstLetter() and getLastLetter() since much of getLetter() is taken 
up with the conditionals and compensating for trying to do two things.

def getFirstLetter(string, number=0):
     if string[number].isalpha() == True:
         return (number, string[:number + 1])
     else:
         return getFirstLetter(string, number + 1)

Even better would be to use a simple loop to find the index of the first 
letter, and split the string into three sections in the caller.

BTW thinking of writing this as a loop brings to mind some problems - 
what will your program do with 'words' such as 555-1212 or "Ha!" ?

>     if direction == 'forward':
>         increment = 1
>     else:
>         increment = -1
>     if string[number].isalpha() == True:
>         if direction == 'forward':
>             return (number, string[:number + 1])
>         else:
>             return (number, string[number:])
>     elif string[number].isalpha() == False:
>         return getLetter(string, number + increment, direction)
> 
> 
> if __name__ == "__main__":
>     try:
>         if sys.argv[1].isspace() == True:
>             print "No file was given to the program to 
> process.\n<----------> Program quitting <---------->\n"
>         else:
>             try:    
>                 f = open(sys.argv[1])
>                 fileScramble(f)
>                 f.close()
>             except IOError:
>                 print "That file does not exist, or you do not have 
> permission to access it.\n<----------> Program quitting <---------->\n"
>     except IndexError:
>         print "No file was given to the program to 
> process.\n<----------> Program quitting <---------->\n"
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> Tutor maillist  -  Tutor at python.org
> http://mail.python.org/mailman/listinfo/tutor