Geek Answers Handbook

My first python program: can you tell me what I'm doing wrong?

I hope this question is considered appropriate for stackoverflow. If not, I will delete the question immediately.

I just wrote my first python program. The idea is that you can issue a command, and it is sent to several servers in parallel.

This is for personal educational purposes only. The program works! I really want to practice python, and so I would like to ask the following questions:

  • My style looks messy compared to PHP (which I'm used to). You have suggestions for improving the style.
  • Am I using the right libraries? Am I using them correctly?
  • Am I using the correct data types? Am I using them correctly?

I have a good programming background, but it took me a while to develop a decent style for PHP (PEAR coding standards, knowing which tools to use and when).

Source (single file, 92 lines of code)

http://code.google.com/p/floep/source/browse/trunk/floep

+6
python
source share
4 answers

It is usually preferable that the following after the sentence ends : is on a separate line (also do not add a space before it)

 if options.verbose: print ""

instead

 if options.verbose : print ""

You do not need to check the len of the list if you are going to iterate over it

 if len(threadlist) > 0 : for server in threadlist : ...

redundant, more "readable" (python is smart enough not to iterate over an empty list):

 for server in threadlist: ...

Also, more "pythonistic" is the use of list (but, of course, this is a controversial opinion)

 server = [] for i in grouplist : servers+=getServers(i)

can be reduced to

 server = [getServers(i) for i in grouplist]
+10
source share

Before you upload any criticism, first let me congratulate you on how your first Python program works. Moving from one language to another can be a daunting task, constantly struggling with syntax issues and hunting through unfamiliar libraries.

The most cited PEP-8 style directive, but this is only a guide, and at least part of it is ignored ... no, I mean, this does not apply to any particular situation, with all due respect to the authors and participants of the recommendations: -).

I cannot compare it with PHP, but compared to other Python applications it is pretty clear that you follow style conventions from other languages. I did not always agree with many things that other developers said that you should do, but over time I realized why using conventions helps to communicate what the application is doing and will help other developers help you.

Raise exceptions, not rows.
  raise 'Server or group' + sectionname + 'not found in' + configfile

becomes

  raise RuntimeError ('Server or group' + sectionname + 'not found in' + configfile)

There is no space before the ":" at the end of the "if" or "for" and do not put multiple statements on the same line and do not agree to put spaces around the statements. Use variable names for objects and stick to i and j for loop index variables (for example, our FORTRAN workshop ancestors):
  for i in grouplist: servers + = getServers (i)

becomes:

  for section in grouplist:
     servers + = getServers (section)

Containers can be tested for content without getting their length:
  while len (threadlist)> 0:

becomes

  while threadlist:

and

  if command.strip () == "":

becomes

  if command.strip ():

Separation of a tuple usually does not fit in brackets on the left side of the statement, and the command logic is a bit confusing. If there are no arguments, then .join (...) will be an empty string:
  (options, args) = parser.parse_args ()

 if options.verbose: print "floep 0.1" 

 command = "" .join (args)

 if command.strip () == "": parser.error ('no command given')

becomes

  options, args = parser.parse_args ()
 if options.verbose:
     print "floep 0.1" 

 if not args:
     parser.error ('no command given')
 command = "" .join (args)

Python for the loop has an unusual else clause, which is executed if the loop goes through all the elements without a break:
  for server in threadlist:
         foundOne = False 
         if not server.isAlive ():
             ... snip ...
             foundOne = True

         if not foundOne:
             time.sleep (0.010)

becomes

  for server in threadlist:
         if not server.isAlive ():
             ... snip ...
             break
     else:
         time.sleep (0.010)

Getting a list of strings and then combining them back is a little longer:
  result = proc.readlines ()
         strresult = ''
         for line in result: strresult + = line 
         self.result = strresult

becomes

  self.result = proc.read ()

Using the library is useful, check the subprocess module, it is slightly updated.

Your data types are fine.

And you will get many other anwsers :-)

+7
source share

String exceptions are deprecated in Python, so this string:

 if not config.has_section(sectionname): raise 'Server or group ' + sectionname + ' not found in ' + configfile

should be remade into something like this:

 if not config.has_section(sectionname): raise ConfigNotFoundError( "Server or group" + sectionname + "not found in" + configfile) class ConfigNotFoundError(Exception): pass

[Edited to reflect dangph's suggestion in the comments]

These are more lines of code, but better for future updates.

For readability, something like this:

 parser.add_option('-q','--quiet',action="store_false", help="Display only server output", dest="verbose", default=True)

You can rewrite it like this:

 parser.add_option('-q', '--quiet', action="store_false", help="Display only server output", dest="verbose", default=True)

You may prefer a different method for splitting a method call, but the idea is that long lines can be hard to read.

You can also read PEP 8 to understand the Python style.

+5
source share

Often for reuse we do the following, starting at about line 48 in your program

 def main(): config = ConfigParser.RawConfigParser() etc. if __name__ == "__main__": main()

This is just a starting point.

Once you do this, you realize that main () actually consists of two parts: parsing the command line interface and doing the job. Then you want to reorganize everything to look like this.

 def serverWork(group,...): servers = getServers(group) etc. def main(): config = ConfigParser.RawConfigParser() if command.strip() == "": parser.error('no command given') else: serverWork( options.group, options.etc., ... )

Now you have raised the real work to a function inside this module. Your serverWork function can now be reused by other programs or scripts.

+3
source share

More articles:


All Articles