A suggestion for a possible Python module

[Max M]

Lance LaDamage wrote:

> ([driveletter]:\[pythondir]\Lib). It is located at
> http://sourcepost.sytes.net/sourceview.aspx?source_id=4105. Tell me what you
> think of it.


Oh ... it just dawned on me that this might not be an attempt at humour. 
So my humorous reply earlier today might have been rather out of line.

But the code was so full of errors as to be a guidebook of bad 
programming practices that I assumed it to be a humorous post.

Instead I will do a "serious" analysis of the code. I assume you are a 
new coder, if not some of my comments might be obvious::

First of, it is overly commented. You take 3 lines of actual code, well 
one line in the comming version of python, and turns it into 36 lines. 
That is not nessecary.

Here is the relevant code:

     import string

     # This is the reverse function
     def reverse( s ):
         if len( what ) != 1: # If the query is not 1 character long
             what2 = list( what )
             what2.reverse()
             return "".join( what2 )
             return
         else: # If the query is one character long
             return "I can't reverse one character"
             return

     def help():
         return "Use StrReverse.reverse(s) to reverse a string"

     def credits():
         return "Special Thanks to Lance L. LaDamage for making the \
StrReverse module"


First of you import the string module, but you don't use it anywhere. I 
assume you used the reverse function in the string module to begin with, 
and then found out about the string method, and then forgot to delete 
the import. While not harmfull it is sloppy coding.

     import string

Also you don't follow the python coding standard by having spaces in 
your function calls.

     reverse( s )

It should look like:

     reverse(s)

"This is the reverse function" is an example of really bad commenting. 
You are commenting the obvious. It is just a textual repetition of the 
function definition.

     # This is the reverse function

Instead you should state the intent of the function. Also you should do 
it as a docstring instead.

     def reverse( s ):
	"Reverse the ordering of letters in a string"

You call your parameter "s". This is not too bad, but perhaps "aString" 
would be a better name. At least it is a lot more descriptive.

Here follows the worst mistake. It is a plain bug! If your string has 
only one letter, it will not try and reverse it. Instead it will return 
another string "I can't reverse one character". This is dangerous in so 
many ways!

If you really want it to be an error to try and reverse a 1 char string, 
you should raise an exception. Not let the error propagate throughout 
your software system. You risk inserting "I can't reverse one character" 
strings all over you database or whatever.

         if len( what ) != 1: # If the query is not 1 character long
             what2 = list( what )
             what2.reverse()
             return "".join( what2 )
             return
         else: # If the query is one character long
             return "I can't reverse one character"
             return

But not reversing one letter strings is a bad idea. It would be 
apropriate to expect a reverse function to be reversible.

So that if I tried:

     print reverse(reverse('Lance'))
     >>> Lance

Which it would do correctly. But why should this be wrong?:

     print reverse(reverse('L'))
     >>> I can't reverse one character

In the end of your "if" and "else" statements, there are superfluous 
return statements that will never be executed. Why is that?

Then there is the "what" and "what2" variables. Those names are not 
descriptive. Furthermore they are not declared anywhere, and so will 
generate an error when the function is called.

Have you even tried to run the code you have posted?

All in all you the function should probably look something like:

     def reverse(aString):
	"Reverse the ordering of letters in a string"
         list_of_letters = list(aString)
         list_of_letters.reverse()
         return ''.join(list_of_letters)

It would have all the comments you need, and properly, descriptive named 
variables.

-- 

hilsen/regards Max M Rasmussen, Denmark

http://www.futureport.dk/
Fremtiden, videnskab, skeptiscisme og transhumanisme