in my last article i was stressing the importance of testing code with realistic data volumes in order to detect potential performance issues when the code is actually deployed. that got me to thinking about something that i don't normally think about. the quality of the code we write. now you are probably wondering what on earth i mean by the "quality" of the code that we write? after all, code is either good (it works, does what it is supposed to and doesn't crash) or bad (it doesn't do what it is supposed to, or crashes). however, that addresses only the functionality of the code, not its quality.

one of vfp's greatest features is that there are almost always several different ways of doing the same thing. unfortunately, this is also one of the worst things about the language because, if there were only one way of doing something, you would have no choice. what i notice, whenever i am working on code (my own or someone else’s) is how easily we developers fall into patterns of doing things. once we have figured out the way to tackle some particular type of operation we tend to stick to it unquestioningly. i believe there are three critical issues that affect the quality of our code:

  • maintainability    ask yourself this question. could someone (assuming they are a technically competent developer) take over your maintaining and enhancing your code? is there (current!) design and implementation documentation explaining what the code is supposed to do and how it actually does it? is the code itself commented so that it is clear what is going on at every point?
  • efficiency           code can easily be functionally correct, but perform sub-optimally. the vacation calculator that took 45 minutes to run (that i referred in my last article) is a perfect example of code that worked but was not optimal. as noted, determining whether the code is optimal is largely a matter of proper testing at each level, but that is detection after the fact. what can you do to avoid writing sub-optimal to start with?
  • best practices    this is probably the hardest one of all. what are the "best practices' for vfp code? who decides what they are, and where are they defined? the only answer to these questions is that i don't know.

therein lies the problem; there are no absolute rules that can be applied to all of these issues, in all cases. as with so many things it is a largely a matter of personal judgment and preference. what i can do is to offer some suggestions, based on my 20+ years of working with foxpro in its various incarnations (if there are mistakes to be made, you can bet that i've probably made them all at one time or another). so here goes with my top ten rules for writing better code …

rule #1: never copy and paste code     since every piece of code is different - in either intent or implementation (or both), there can never be a situation where you should copy and paste code that was written elsewhere. if you do find yourself copying and pasting then it means that you are in one of two situations. first you have an operation which closely mimics an existing operation. in this case you should probably be designing a more generic solution to handle both operations rather than trying to hack existing code. or second, you are simply duplicating functionality that already exists elsewhere because, in its current form or location, it cannot be accessed directly. in this case the code should not be duplicated but should be extracted and made available as either a method or procedure to everything that needs to use it.

rule #2: don't adopt global solutions to solve local problems there are basically two types of environmental control commands in vfp; those that are scoped to the datasession (e.g. set exclusive and set deleted) and those that are global to vfp itself (e.g. set escape and set nulldisplay). in either case these commands are actually global – they affect everything within their scope and so should only be used when you really need to change the working environment as a whole. consider the following code (from a real application i may add) whose intent was to un-delete records previously flagged for deletion:

procedure <name>
use <table> order <tagname>
set deleted off
set filter to deleted()
***
***** functional code here
***
set deleted on
return

now, what's wrong here. first, there is absolutely no error checking at all and the use command is poorly implemented, at the very least an "in 0" should have been used to avoid inadvertently closing an already open table. however, the bad part is the explicit use of the global set commands. irrespective of whatever else is going on in the application, all tables in the current datasession will be running with deleted = on after this procedure is called. while that is fine when it is the intended behavior, it shouldn't happen by chance. at the very least, the setting should have been checked on entry to the procedure and restore to whatever state was extant at the end. but the real issue is why even use a global command at all? if you want to un-delete records, why not just use recall for <condition>? it doesn't matter to the recall command where deleted is set on or off and there is no need to mess with a global setting at all.

rule #3: ensure that return values are meaningful      it is often said that the difference between a procedure and a function is that a function returns a value. however, in vfp this is not actually true because the only difference between a procedure and a function is how the code is called, and whether parameters are passed by default by reference (in a procedure call) or by value (in a function call). what this means, therefore, is that all vfp procedures, functions and methods must always return a value to the calling code (if nothing else is specified the value is simply a logical .t.). any code that might be interpreted as a function must, therefore, return a meaningful value. here is an example of code that, in the application, is called using the old "=" syntax (i.e. =opendbf('table_name'))

****************************************
function opendbf
parameter tablename, ordertag
****************************************
if not used(tablename)
  select 0
  use (tablename)
endif
select (tablename)
if parameters() = 2 and not empty(alltrim(ordertag))
  set order to tag &ordertag
endif

notice that there is no explicit return value so this "function" will always return "true" – even after an error. what is needed here is some local error trapping (try…catch was introduced into the language for precisely this kind of situation) and a proper return value that informs the calling code whether the function achieved its objective, or not, so that the calling code can take the appropriate action.

this leads directly to rules 4 and 5:

rule #4: always include an explicit return statement            the reason for this rule is that if you omit the return statement, and the last line of the procedure is a function call, there is no way to see the result in the debugger. for example, when debugging the procedure code above unless you pass the second (ordertag) parameter the last line of code that you can step into is the "select (tablename)" statement. by including the return command you provide a stopping point for the debugger so that you can evaluate the results of the last block of code too!

rule #5: always check the return value from a function call     possibly the worst example of violating this rule comes from the vfp help file itself. in the tableupdate() topic is the following "example":

replace clastname with 'jones'
? 'new clastname value: '
?? clastname  && displays new clastname value (jones).
= tableupdate(.t.)  && commits changes.
? 'updated clastname value: '
?? clastname  && displays current clastname value (jones).

this example has caused more developers more grief and prompted more questions on technical forums than any other single item in the help file. typically you see something like this:

bug: tableupdate not working!

my save button calls tableupdate and there is no error, but when i look in the table the changes aren't there. what's wrong?

the answer is almost always that the developer based their code on the help file example! but as stated, quite clearly in the help topic (under 'return value'), tableupdate() like all vfp functions returns a value. if the update succeeds, the return value is true; if it fails, no error is generated but the return value is false and the windows error structure is populated. the vfp aerror() function can be used to determine the actual error message. so the correct way to use the tableupdate() function is:

replace clastname with 'jones'
? 'new clastname value: '
?? clastname  && displays new clastname value (jones).
if not tableupdate(.t.)  && attempt to commit changes
  aerror( laerr )
  messagebox( laerr[2], 16, 'update failed' )
else
  ? 'updated clastname value: '
  ?? clastname  && displays current clastname value (jones).
endif

any time i see code that uses "=functionname()" i am immediately suspicious. what this is doing is saying that the result of the function call is irrelevant, and while that may be true in certain cases, it is certainly not the norm and is certainly not good practice.

rule #6: always check, and validate, input parameters this may seem obvious but, as shown in the opendbf() function above this fundamental rule is not always followed. my own preference is to use code like this to validate parameters and while there are many different ways of handling the issue, handled it must be!

function addconnection( tuconname )
  *** check the input parameter
  if vartype(tuconname) <> "o"
    *** we didn't get a connection object
    if vartype( tuconname ) = "c" and ! empty( tuconname )
      locondets = .getconnection( tuconname  )
      if not locondets.lstatus
        *** could not find this connection so just return
        return .makeresobj( .f. )
      endif
    else
      *** invalid or no connection name passed
      this.logerror( 9014, tuconname, lower( program()))
      return .makeresobj( .f. )
    endif
  else
    if lower( tuconname.class ) <> "xconnection"
      *** invalid or connection object passed
      this.logerror( 9014, tuconname, lower( program()))
      return .makeresobj( .f. )
    endif
    *** if we get to here we have a valid connection object

  endif

as you can see, this code handles the case where an object of a specific type is expected, but where the name of an object may be passed instead of the reference. either case is catered for and, while the code could be condensed it is both readable and unambiguous in this format. which leads me directly to rule 7.

rule #7: don't use 'cute', but obscure, coding  i, like most people, like others to think of me as being "clever" and i regard that as a natural enough desire. however, there is no excuse for projecting that desire into code so that it becomes obscure and difficult to understand. consider the following piece of code:

do case
  case ! checkparameters( tcname, tlstatus, tdlastdate )
    lcerror = "invalid parameters:
  case ! updatename( tcname )
    lcerror = "cannot update the specified name value"
  case isdateoutofrange( tdlastdate )
    lcerror = "date is outside the allowable range"
  case ! setstatus( tlstatus )
    lcerror = "cannot update the status flag"
  otherwise
    lcerror = ""
endcase
if not empty( lcerror )
  messagebox( lcerror, 16, 'error occurred' )
endif

what this code is doing is simple enough – it is using the case construct to call each function in turn because, in each case, the function is expected to return a result that means that the case condition evaluates as false. this forces the next case to be evaluated, and so on. however, this is not immediately obvious, and is certainly not a  normal use of the do case construct. especially in the absence of any comments it would very be hard to pick this out of a program and understand it without considerable effort. similarly code like this little function, also cute, is hardly readable, let alone maintainable:

lparameters tnday
local lcday
*** convert day number to the day of the week
lcday = iif( tnday = 1, 'sunday', iif( tnday = 2, 'monday', iif( tnday = 3, 'tuesday', iif( tnday = 4, ;
'wednesday', iif( tnday = 5, 'thursday', iif( tnday = 6, 'friday', iif( tnday = 7, 'saturday', ;
'invalid day number')))))))
return m.lcday

and here's another example – which would you prefer to run across in code you have to debug?

mimg = iif(file(sys(5)+sys(2003)+"\img\"+ m.ic +".jpg",sys(5)+sys(2003);
+"\img\"+ m.ic + ".jpg",sys(5)+sys(2003)+'\img\noimg.jpg')

or

if file(sys(5)+sys(2003)+"\img\"+ m.ic +".jpg")
  m.img = sys(5)+sys(2003)+"\img\"+ m.ic + ".jpg"
else
  m.img = sys(5)+sys(2003)+'\img\noimg.jpg'
endif

in each case the result is the same, the specified image file is checked for, and if found assigned to the variable, otherwise a default image is assigned instead. but, in my opinion clarity ( and hence maintainability) is more important than merely saving a couple of lines of code.

rule #8: don't create unnecessary functions    what on earth is an "unnecessary" function? simply it is a function whose only functionality is to call other functions. this example is from some code that someone sent to me recently:

if not isallupper( string )
  string = upper( string )
endif

what, i wondered, was the "isallupper()" function call for. it's not a vfp function so it had to be a udf and, opening the code, this is what i found:

*************************************************
function isallupper
*************************************************
parameters string
if upper(string) = string
  return .t.
else
  return .f.
endif

this is totally absurd since there is no conditional logic here at all. the result is to always force the string to upper case, so the only code needed is:

string = upper(string)

here's another example:

function cityline
lparameters lccity, lcstate, lczipcode
f = alltrim( lccity ) + ", " + alltrim( lcstate ) + " " + alltrim( lczipcode )
return f

since all this is doing is concatenating the three input parameters, the code could just as easily concatenate them directly. admittedly if this is done often and there is a possibility it might have to change, then there is some merit to the function (it concentrates the code in one place), or if there was some other variable that applied some other logic (country-specific formatting for example). however, as it stands it is clearly an unnecessary function and achieves nothing more than to introduce additional overhead into the program.
rule #9: don't use magic numbers       a 'magic' number is simply some undefined value which is used to interpret data in some way. again, from a real application i got the following piece of code:

if m_userlevel < 20
  if m_userid = m_owner
    m_userlevel = 11
  else
    m_userlevel = 12
  endif
endif

that is all that there was! now, by interpreting what the code was actually doing i was eventually able to figure out that userlevel 11 was "executive" and userlevel 12 was "other manager". but there were no other numbers used (anywhere in the code) and there was no definition – either in code, an include file or in a table – of what these numbers meant. if you need values like this they must be defined in a table somewhere. that immediately conveys four benefits. first, you won’t forget what they mean! second if you need to change the descrptions for these values, then you don’t need to change any code, you simply change the values in the table. third, you have a consistent set of definitions which allow you to display meaningful text without having to resort code like this:

do case
  case lnuserlevel = 11
    lcdisplayname = ‘responsible executive’
  case lnuserlevel = 12
    lcdisplayname = ‘manager’
  otherwise
    lcdisplayname = ‘unknown user’
endcase  

and finally you have a way to interpret the values for those situations when you don't have the ability to write the code directly (like in a report, or an extract file for example).

rule #10: name methods and functions appropriately this is another 'obvious' one! however, yet again, it is easy to find examples that show that people don't  do it. here is an example:

if ischange( m.username )
  replace username with m.username in loginusers
endif

now what would you expect this code to be doing? when i first saw it i assumed that it meant that if the user name has been changed, then update the "loginusers" table with the new user name. however, in the context of the application that didn't seem to make sense, so i went looking. here is the code called by ischanged():

procedure ischange
parameters string
return change2lower(string)

procedure change2lower
parameters string
string = alltrim(string)
retstring = ""
do while not empty(string)
  * raise first char as upper case
  string = upper(substr(string, 1, 1)) ;
         + iif(len(string) > 1, substr(string, 2), "")
  retstring = retstring + getfirstword(string) + " "
  string = removefirstword(string)
enddo
return retstring

if you examine this you will see that what it does is actually to capitalize the first letter of each word in the input string and put the rest into lower case. so not only are these examples of unnecessary functions - the actual code required is just

replace username with proper( m.username ) in loginusers

but the names used are totally inappropriate. since all ischange() does is to call change2lower() why is it named "ischange"? moreover, change2lower() does not change a string to lower case, it changes it to proper case.

how about this one, a program file named "productn.prg"? what does it do? well, here's the only comment from the code:

* productn - check for duplicates

hardly what you would call a descriptive name for the program!

there are lots more things that could be included, but these are my current list of the 'top 10' rules for writing code:

rule #1: never copy and paste code

rule #2: don't adopt global solutions to solve local problems

rule #3: ensure that return values are meaningful

rule #4: always include an explicit return statement

rule #5: always check the return value from a function call

rule #6: always check, and validate, input parameters

rule #7: don't use 'cute', but obscure, coding

rule #8: don't create unnecessary functions

rule #9: don't use magic numbers

rule #10: name methods and functions appropriately

i'd be interested ot hear from anyone with their list....

7 Responses to Best Practices for Coding

  • PIlotBob says:

    I agree with rule number 7 but I totally disagree with your example of using the case statement to implement a template pattern is “cute” and/or “obscure”. It is extremely logical and reads very well… much better than the alternative of 6 full if/then statements…

    Read it out loud to yourself in english:

    In the case that CheckParameters() fails there are invalid parameters and stop processing.

    How is that obscure or cute I don’t know?

    BOb

    I only meant that it is “cute” because the normal usage for DO CASE is to execute one, and only one, of the various options. It is also obscure in the sense that it would require a second look (at least) to realize that it is not actually doing that. Just my opinion, of course.  — Andy

  • Victor Savitskiy says:

    I think that disputed code, to do not look obscure, cold look like that. Code to restore original user name if status update has failed, intentionally omitted.

    IF CheckParameters( tcName, tlStatus, tdLastDate )
      IF IsDateIsInRange( tdLastDate )
       
    IF UpdateName( tcName )
         
    IF SetStatus( tlStatus ) 
           
    lcError = “”
         
    ELSE && SetStatus
           
    lcError = “Cannot update the status flag”
         
    ENDIF && SetStatus
       
    ELSE && UpdateName
         
    lcError = “Cannot update the name to ” + tcName
        ENDIF && UpdateName
     
    ELSE && IsDateIsInRange
       
    lcError = “Date is outside the allowable range”
     
    ENDIF && IsDateIsInRange
    ELSE && CheckParameters
     
    lcError = “Invalid Parameters”
    ENDIF && CheckParameters

    IF

    NOT EMPTY( lcError )
      MESSAGEBOX( lcError, 16, ‘Error Occurred’ )
    ENDIF

    Exactly! I agree that it is much clearer and less likely to misinterpretation when written out that way. Interestingly it may even execute faster (nested IF is usually faster than a DO CASE) — Andy

  • Victor Savitskiy says:

    I would also add to the list.

    #4a: always have single RETURN command at the end of code (method, function, program, procedure). I used to work with multiple RETURN until some C compiler warned me about that. This warning made my work much easier and organized.

    #5a: never make a call like DO Subroutine if you can use IF Subroutine() or SubroutineResult = Subroutine(). If you must use DO Subroutine use it as DO Subroutine WITH @SubroutineResult.

    #11: When you work with form method, try to read all important for this method data into local variables first and then – manipulate with these variables. The method code does not depend anymore on the way how data appeared here (form property, parameter, application object…). You will also avoid using obj.prop notation inside of SQL statement.

    All good stuff. Thanks — Andy

  • Mike Potjer says:

    This is just a correction, but the example for Rule #6 is a good illustration of Rule #1. 🙂
    There are references to .GetConnection() and .MakeResobj(), but neither of these method calls include the object reference, nor is there a WITH structure around them.

    Well spotted. Oddly enough this is not actually the complete code, just a part that I lifted for illustrative purposes. But then you knew that Smile <img src=” /> — Andy

  • Victor Savitskiy says:

    Andy,

    I did not point to it explicitly, but you might have noticed that my nested IF sample re-write includes one more rule: “Positive thinking”, which helps with programming, too. The original sample had been programmed mostly negatively – DO CASE branches have CASE NOT except one: CASE IsDateOutofRange( tdLastDate ), which is written using mixed logic – if date is wrong, function returns true! Using negative logic sooner or later will confuse even the original programmer (not to speak about others) with multiple negations.

    There are some cases, when code below technically will work without consequences,

    IF Cond1
       RETURN
    ENDIF

    IF Cond2
       RETURN
    ENDIF

    but it works only if there is nothing to do at the end of code. Also – there is another way to avoid this – check the condition in the caller code.

    Yes, you are right. Mixed logic always has the potential for confusion. I generally try to test for the positive and handle the negative in the ELSE condition too. — Andy

  • TOm Knauf says:

    Andy,

    very good stuff.

    I would add just one :

    Decide to use a naming convention for  variables, function, objects,… and stay with it in the whole company.
    Do never mix languages (nSchl / nrun) or
    singular / plural or prefixes (cName / lcName)

    Thank you Tom, good pointers for anyone’s coding rules and standards. I’ll be up to 20 Rules any day at this rate. — Andy

    Best regards
    Tom

  • Bob Ethridge says:

    When creating external programs to be used from within an existing application, consider defining a session class.  This is a cautious and easy approach to prevent problems opening and closing tables that are already in use in another session. I’m keeping this to a simple example that doesn’t include buffering or SET’s, and assumes that nulling the object will close the session.  All of these factors could be defined in the class procedures as well, or even better, be subclassed from a main session class belonging to the application.

    ***************************************

    PARAMETERS tnThreshold as Integer

    LOCAL loSessionExample AS ‘mySessionExample’ OF ‘CityThreshold.prg’, ;

    llOK AS Boolean

    llOK = .F.

    IF VARTYPE(tnThreshold) = "N"

    IF ALLTRIM(UPPER(JUSTSTEM(DBC()))) == ‘MYDBC’

    loSessionExample = CREATEOBJECT(‘mySessionExample’, DBC())

    WITH loSessionExample

    .nThreshold = tnThreshold

    llOK = .PROCESS()

    ENDWITH

    loSessionExample = NULL

    ELSE

    MESSAGEBOX(‘myDBC is not the Current Database’, ;

    16, ‘Unexpected Error’)

    ENDIF

    ELSE

    MESSAGEBOX(‘The Threshold Parameter was not passed.’, ;

    16, ‘Unexpected Error’)

    ENDIF

    RETURN llOK

    DEFINE CLASS mySessionExample AS SESSION

    CAPTION   = ‘Session Example’

    nThreshold = 0 && distinct city/state

    PROCEDURE INIT (tcDBC AS STRING)

    OPEN DATABASE (tcDBC)

    ENDPROC && init

    PROCEDURE PROCESS

    LOCAL llOK AS Boolean

    WITH THIS

    SELECT DISTINCT state, city ;

    FROM myDBC!Address ;

    INTO CURSOR csrAddr

    llOK = _TALLY >= .nThreshold

    ENDWITH

    RETURN llOK

    ENDPROC && process

    ENDDEFINE && mySessionExample

    ***************************************

Leave a Reply

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