Just because someone fancies him/herself a competent programmer does not make it so!

While updating an application we inherited recently, Jeremy (one of our programmers) emailed me this example.  (This happens to be Visual FoxPro code, but we often encounter similar programming in Visual Basic, C++, and PHP code we inherit…)

This is really a circuitous way of performing a simple function.

This is really a circuitous way of performing a simple function.

“OK, Dave,” you’re probably saying–what’s so bad about it?  Well, a few observations come to mind:

1) Even with the comment above this code block, it’s difficult to understand what in the world the programmer was attempting to do.  There are much better ways to strip embedded spaces from fields.

2) The code uses single-letter variable names.  Suppose the program bombs, returning an error  message such as:  “Variable N not found.”  Guess how many times the letter ‘N’ occurs in this program?  How long will it take to find the specific ‘N’ being referenced?   Better:  use Hungarian notation.  For example:  lnX = AT(‘ ‘, … etc).   It’ll be a whole lot easier finding lnX in the code than trying to locate the specific instance of ‘N’ that caused the error.

3)  This code will write and rewrite strings to the database up to 20 times per record–when a single write per record would suffice.

4) This logic, spanning nine lines of code, creates a nested looping structure.  Nine lines will be much more difficult to maintain (and debug) than what’s needed.

So, how would we recommend changing the coding above?  How about a single line of code: 

UPDATE <table> SET ponumb=STRTRAN(ponumb,’ ‘) WHERE AT(‘ ‘,ALLTRIM(ponumb)) > 0

An added bonus:  this logic will only write to the database when a PO number actually contains embedded spaces.  The fewer times our code updates the database, the better.

Leave a Reply

Your email address will not be published. Required fields are marked *