Coding standard for Clipper

Using Fivewin

Author: Ran van den Boom
e-mail: r.vd.boom at casema.nl (replace at with @)

This Coding standard is developed while working at Decos Software Engineering bv, The Netherlands
www.decos.com; www.decos.nl

Nowadays I work as software project manager, developing statistical software with teams varying from 2 to 20 team members.

Shortcuts: Table of contents, Code review checklist

Introduction

This coding standard should be used to improve the quality of the software product written in Clipper. Since each has his or hers own programming style, this coding standard should not be used to check this style, nor to check each character of the code. Instead, agree upon the issues that are relevant for your project. Together with the development team choose between the possible coding styles. Use the parts that are relevant given the situation the project is in.

Using this coding standard itself does not result in a good functioning program. Design is the first thing to do well. Without attempting to describe how to make a good design, I have added some indications of poor design. First however the naming conventions are described, and the way to structure the code itself.

This document is based on ten years of experience with Clipper and five years experience with Fivewin, and the experience of using a coding standard for VB, C and C++ projects. In all these projects deciding upon the coding standard is one thing, using it and reviewing code is essential. Therefore I have added several suggestions, a brief explanation on how to perform a review and a code review checklist. For a background of why this coding standard is developed, see Clipper coding standard background.

Aside from the coding standard for our projects we use a description of the development environment and a description of the files used, their contents and a global function explanation. Especially the list of general-purpose functions is very handy. Programmers that are new to the project tend to write their own functions instead of re-using the existing ones. For each project this coding standard is used, add a reference to the development environment description and the code description.

Table 1: Development environment and source description

Updates

The coding standard comprises two parts: the coding standard explained and a code review checklist.

I sometimes update this coding standard based on the reactions. You can send your comments to the email address above.

Table of contents

Coding style rules, naming conventions
* Naming scheme
* Code readability
* Source file layout
* Other layout guidelines
Code construction
* Structured programming
* Always use all parameters
* Use defines
* Use the command line syntax
* Error checking
* Useful functions and names
* Button function names
* Free resources as soon as possible
* Adding functions
* Static functions
* Comparing strings
* Order of DBCOMMIT and DBUNLOCK
* Bounds checking
* Other considerations
General coding suggestions
* Memory usage
* Variables scope
* Filters versus scope
* Use of SysRefresh
* Maximum file size DBF and DBT
* File function
Enforcing the coding standard
* Tools
* How to review
* Code review checklist
Appendix Example
Appendix Debugging using Monitor statements without levels
Appendix Debugging using Monitor statements with levels
Appendix: File functions
Remark list template
More information
* Clipper
* Fivewin
* Other coding standards

Coding style rules, naming conventions

Naming scheme

Variables

Only simple counters may use variables like i, n; however a more proper name should be used in new code.
This naming scheme looks somewhat like the Hungarian naming scheme for variables used in C.

Constants, defines

Strings

Numeric values

Other

The NIL_ define is used to clarify NIL parameters in calls to Fivewin functions such as:
MsgWait( MSG_PROCESSINGDATA, MSG_WAIT, NIL_SECONDS )
This clearly shows what is the purpose of the third parameter.

Functions

Use a prefix of the module or the class for a function name. So that it’s clear where the function can be found (module, class), where it’s related to. Also the first character of each word within a function is written as a capital, the rest of the word is not in capitals. For instance:

search_next_string(cStr) /* Wrong */
SrchNextStr(cStr) /* Ok */

Since Clipper only uses the first ten characters of function names and names of variables, using long names is not useful. A function should be named <module><object><action>. For example:

lOk := ObtainLinkWithAddressesAndAddressBooks(cAddressKey) /* Wrong */
lOk := AdrLinkGet(cAddressKey) /* Ok */

The documents in Table 1: Development environment and source description contain a description of the currently used prefixes.

Code readability

Why is code readability such an important issue in this coding standard? First of all if a team of programmers is working in the same source code files, you need to have an agreement on how the layout of the code should look like. Even if you agree to don’t have general code layout, that’s still an agreement. This coding standard has got a lot of agreements about coding style and you can decide per item to use it or not.

Second, the better the code is readable, the better can be maintained. This reduces the chance of bugs and simplifies debugging the code and fixing the bugs. In our experience there are a lot of bugs in the source code and not only in Microsoft products.

Remove unused code

Code indicated with #ifdef NEEDED or #ifdef OLD, or code commented out can be deleted. We make a backup with each release so if it was important code after all: blame the one who wrote the #ifdef or commented out the code. If you want to rewrite a function but the old code must be available for backward compatibility, rename the function by adding "Old" in front of the function name.

Also remove defines, includes, MEMVARs, etcetera if they are not used.

Clipper in uppercase

To distinguish Clipper statements from calls to own functions, uppercase Clipper statements, commands and functions are used. Note however that alternatives are also possible, as long as you can distinguish Clipper statements from other ones.
Example:
IF !EMPTY(cSrchString)
...AADD(aActiveFlds, cSrchString)
ENDIF

Fivewin internal code uses another scheme:
if ! ::lBorder
..::lBorder = .t.
..::Refresh()
endif

Yet another alternative is
If !Empty(cSrchString)
...Aadd(aActiveFlds, cSrchString)
EndIf

In all cases an editor with syntax highlighting such as Ed4Win proves to be very useful.

Source file layout

The source file should first contain the definitions (such as defines, xtranslates, xcommand, MEMVARs) that are located in other files, then local definitions.

• Header
• Include files
• MEMVARs
• STATICs
• Defines
• Language defines
• Code

The language defines look like:

#ifdef ENGLISH
...#define STR_HELLO "Hello"
#else
...#define STR_HELLO "Hallo"
#endif

File header

/****************************************
<Project name>
<Description of source file>
Author(s) : <author(s)>
(C) Copyright <year>-<year>
<Company name>
****************************************/

The years behind Copyright are <year the project is started>-<current year the modification is made in>. For other projects similar file headers can be used.

Header of function

Agree upon the template to use for function headers. Example of such a header:

/*=========================================================================*/
Name: SrchGetSrchColumns
Description: Return the columns to search in
Parameters in: cFileType - type of file (e.g. addresses, documents);
cFileKey - key of the file
Parameters out: array with the descriptions of the columns, empty array on error
Global variables used: none
/*=========================================================================*/

You should prevent to place information that is not relevant in the header. For example repeating the function name plus parameters does not add new information since the function is just below the header.

Other layout guidelines

• Place .AND. and .OR. at the end of the line:
  IF aSetting[SHOW_WHEN_LOADING] .OR. ;
  ...aSetting[SHOW_AFTER_LOADING]
  is preferred over
  IF aSetting[SHOW_WHEN_LOADING];
  ....OR. aSetting[SHOW_AFTER_LOADING]

• Behind each #endif, ENDIF, NEXT, END, ENDDO etcetera, repeat the loop criteria. For instance:
  ENDDO /* WHILE !EOF().AND.!lFound */

• Use spaces before and after assignment operators:
  cKey := X->ADR_CODE

• Either use or don’t use spaces before and after "(" and ")":
  cKey := BookKey(cBookName)
  Alternative:
  cKey := BookKey ( cBookName )

• Add or do not add empty lines before and after Clipper statements:
  IF lOk
  ...lOk := SaveSelection(cUser, cDescription)
  ENDIF
  
  IF lOk
  ...oDlg:End()
  ENDIF

Code construction

This chapter contains guidelines on how to structure the Clipper code.

Structured programming

In Clipper there are a number of possibilities that will make your code less structured. And even if you do not use these commands, it is still possible to write an unstructured program. To reduce the risk of bugs that can result from unstructured programming, the following restrictions and guidelines are meant.

• Use existing functions as far as possible. In our experience when (new) programmers start to work, first thing they do is write their own general purpose functions while the libraries and general purpose functions available should be used.

• Don’t use intermediate returns in a function. It will make the risk of bugs higher, program flow is less clear and the code will be less maintainable. For instance, a function starts by opening two (database) files. Inside the function there are several error checks. On each error the function must return. Big chance closing the files will be forgotten (maybe not by the original programmer but possibly the next programmer isn’t that careful), unless there is only one exit point for the function. At that point the files are closed.

• For the same reasons Clipper statements like EXIT, LOOP should also not be used. BEGIN SEQUENCE … END SEQUENCE is also not to be used.

• Do not use IF ELSEIF ELSEIF ELSE ENDIF if a CASE statement is better. A CASE is better readable. In general a CASE is safer to use in such case because it can be extended easier. The number of IF..ELSEIF..ENDIF, FOR..NEXT, WHILE statements in one function is an indication for the quality of the design; the more of them there are, the worse the design. This is not always true; as said: it is an indication.

• Each CASE statement should have an OTHERWISE statement. If the OTHERWISE should never occur then the OTHERWISE should be used to indicate an error. If the OTHERWISE can occur but nothing should be done then add that to the code in comment, so that it is clear when reading the code.

• Each declaration should be on a separate line: this clearly shows how many variables are required. Variables that only occur in a subsection of the function suggest this subsection can be put in a separate function. Even if that new function is only used inside the function the code originated from it is still worthwhile: the code will be better structured, the scope of the variables is limited to their actual use, code will be better readable.

• If the list of variables is very large, it suggests splitting up the function. The less variables are used, the clearer the code.

• Each initialization and assignment should be on a separate line too. This makes the code more readable.

• The ideal length of a function should be limited to about 60 lines. This number is chosen for then on a print the function will fit on a Letter or A4 size printout. This of course depends on the font size used. If the function length is larger, then it could be an indication the function is not well structured and that it must be split up. It could also be an indication the overall design of that part of the functionality should be rethought.

• The number of parameters passed to a function is an indication of the quality of the design: the more parameters, the worse the design.

• Each function that may return an error should be handled by testing on the possible error. If it occurs, logical lOk is set to .F., Monitor an error, if the user or supervisor can do something about the error show an error dialog with tips to solve the problem, or just show a message indicating why the action is aborted. The next code checks on lOk before continuing.

• Prevent usage of Clipper commands, use functions instead (see also Always use all parameters). Reason is the function syntax shows the parameters used, and the functions can return a value or an error code.
  For example:
  RENAME <cOldFile> TO <cNewFile>
  should be replaced with
  nErr := FRENAME(<cOldFile>,<cNewFile>)
  because FRENAME returns nSuccess.
  Note that there is discussion possible on this guideline. See Always use all parameters.

• Use alias in front of database functions. Since it is not always clear what is the current alias, using the appropriate database alias will reduce the risk of hitting a "Error BASE/1003 Undefined variable" to zero. Disadvantage can be that it slows down the program a tiny bit.
  Example: Use
  CLIENTS->(DBSEEK(cSeek))
  instead of
  SELECT CLIENTS
  DBSEEK(cSeek)

Always use all parameters

In the past on adding parameters they were forgotten ("Hey, they are NIL anyhow, so why bother…") in some of the calls. Then later on, adding yet another parameter, we got a mix up and bugs resulted. Also NIL parameters were not filled in explicitly, and when one of the NIL parameters was replaced with another we got mixed up with all comma’s that were in the call. Therefore I suggest to use the function style using all parameters, and the examples in this coding standard adhere to that.

However there is some discussion on this guideline. Some argue (and they have a point there) that given the object oriented style of object oriented Clipper, the command style resembles closer to overloaded functions. In overloaded functions function A can be called with parameters A en B, but also with parameters A, B and C.

The important point here is to make a consistent choice: either use the command style or the function style, and either use all parameters or some. In my experience, un-experienced programmers tend to forget parameters. This suggests that experienced teams might feel more comfortable with the command style.

One of Clippers strong points is the possibility to use the parameters required and forget about the ones you don’t need. In our experience that is a good thing for the Clipper functions themselves, but for our functions it is best to always supply all parameters, even if they don’t bare relevance in all situations in which case they are passed as NIL. Even better would be to define a NIL_PARAMETER value, to indicate more about the nature of the parameter that should be used in that place. For instance:

FUNCTION NAT(cSearchString, cInString, nTimes)

A function that returns the nTimes position of cSearchString in cInString, can be called with just the first two parameters, defaulting nTimes to one.

can be called by

nAt := NAT("ab", "abcdefgabcdefg")

Better would be:

#define NIL_TIMES NIL

nAt := NAT("ab", "abcdefgabcdefg", NIL_TIMES)

Best would be:

nAt := NAT("ab", "abcdefgabcdefg", 1).

Always use all parameters in calling a function. This is because if the parameter list is extended without adapting the other existing calls an error is easily made.

Function returns NIL if the return value is never used or not relevant. In such case, consider using a procedure instead.

Function returns BOOLEAN if return value is used and difference between .F. and .T. matters.

Use defines

• Use defines for strings. This makes it easier to write and maintain language specific versions. For example
  #ifdef ENGLISH
  ...#define MSG_SEARCHNOTFOUND "Search string not found"
  #else
  ...#define MSG_SEARCHNOTFOUND "Zoek tekst niet gevonden"
  #endif
  Alternative is to have a function that returns the string based on the current language setting. Also use defines in that case. For example:
  #define ID_MSG_SEARCHNOTFOUND 123
  MsgAlert(LangStr(ID_MSG_SEARCHNOTFOUND))

• Use defines for expressions that are constant.
• Use defines from include files if they are already available in include files.

Use the command line syntax

Because in our code we use a lot of defines to support both English and Dutch versions of the program, there is always the risk of a preprocessor overflow. Therefore we have created a new Fivewin.ch to include with lesser defines. This makes us bypass the preprocessor for calls like REDEFINE CHECKBOX ID …; we use TCheckBox():ReDefine(…).
Advantage is that it is clearer which parameters are used and there is less dependancy on include files.
Disadvantage is that if Fivewin modifies its API, we need to change all calls to the modified functions and the function has got a lot of empty or NIL parameters that are now explicitly shown.

Error checking

As much error checking as possible should be programmed. For each function that might return an error, or a non-expected value should check that. Handle errors even if they are not possible to occur, because in the future someone else might use your function the wrong way.

Such errors may not occur very often, but they sure help in debugging faster and future errors will be detected faster. Also external programs that might return an error like the ones started with ShellExecute: if it returns an error code, the code should handle the error.

When opening, creating, closing, removing files as much error checking as possible should be programmed. Use the functions that return an error instead of the commands that do not return an error. For instance: use FRENAME instead of RENAME.

NOTE: Maximum of array length is 4096. Whenever there is a change (however small) that an array can become larger, a test must be added. Note that the DIRECTORY function not always returns a valid array, so in case of using the DIRECTORY function the test on array length must be added for sure.

Useful functions and names

When writing a new function, think of a function name that is unique, covers the functionality of the function and distinguishes it from other function names. If it isn’t, a name might easily conflict with function names already in use or lead to confusion when determining which existing function can be used for a certain functionality.
aArray := GetArray(nType) /* Wrong */
aProcNames := ProcNames(nProcType) /* Ok */

Functions that do not add anything to the code must be removed. Example of such a function:
FUNCTION NumToStr(nVal)
RETURN STR(nVal)
Just use STR(nVal) instead of NumToStr(nVal)

Button function names

Functions used to start a button action start with a "Btn" prefix, to indicate the functions are called from a dialog or window.
Reason is functions like BtnDeleteAddress and DeleteAddress look to be doing the same. However the DeleteAddress function does the deletion and is also called from other functions besides BtnDeleteAddress.

Free resources as soon as possible

Always call free functions as soon as possible. Your memory requirements are reduced, and memory leaks are easier traced. See also Memory usage.

Adding functions

General purpose functions are placed in a number of source files. They are described in documents of Table 1: Development environment and source description. When writing new functions, look carefully where to add them. Combine new functions related to each other, like couplings to an external device in separate source files. Examples are DDE, Scanning, Fax, Word, Excel.

While doing a code review, check whether the functions are in the correct source file and whether the appropriate general purpose functions are used. Examples of such important functions are in Table 1: Development environment and source description.

Some of the source files are very large already, so if you want to add a function in there: think twice whether it should be in there and if so, whether the set of functions related could be part of a new PRG file. Also consider splitting up the source file that is (too) large.

Static functions

Functions should be declared STATIC if the scope of the functions is they are only used inside the file. Note that if a function is passed to an interface function, it can be that the function is no longer STATIC. When a function is part of an evaluation string it must be a global function.

Making functions non-static while they are only local used can cause conflicts with other similar named functions.

Comparing strings

NOTE: comparison of strings should be carefully written!

If the setting SET EXACT is set to OFF,
ALLTRIM("00131")==ALLTRIM("0013")
returns .T. while the texts are not the same. Everywhere in the code there is a risk the string comparison will return .T., use
PADR(ALLTRIM(cStr1),MAX_LEN)== PADR(ALLTRIM(cStr2),MAX_LEN)

MAX_LEN is to be determined based on the context. For instance when comparing ITEM->XINHOUD this test would become:
ITEM->XINHOUD == PADR(ALLTRIM(cStr2),LEN(ITEM->XINHOUD))

Comparison between two strings of fields of type character, the first one with spaces, the second without, gives the wrong result. So if two character fields need to be compared, put PADR around them (if SET EXACT OFF).
The advantage of SET EXACT OFF lies in seeking; if you seek for an address starting with "XYZ", seeking with SET EXACT OFF will find the first address with that characters. Of course provided there is an index on the address.

Order of DBCOMMIT and DBUNLOCK

Committing (DBCOMMIT) needs to be done before unlocking (DBUNLOCK), else another user can write a record or an index before the commit has finished! This results in index corruption, and explains why in test situations you will never get such an error while customers with a lot of concurrent users (so they surely don’t need the index corruption) do get this error when the order is incorrect

When appending a record, check on NETERR()! Don't forget to end a record modification with DBCOMMIT and DBUNLOCK. Else the record stays empty.

Bounds checking

The first and last situation of a loop, the lowest and the highest possible number, both are situations the programmer should be extra careful of. Often such cases are not checked at all for they will never occur. At least, that was what the original programmer might have thought.

The following boundary cases should be carefully reviewed in the code.

• Functions: in FOR .. NEXT and WHILE .. ENDDO loops check the first and last situation in the loop. Are all the variables that are used within the loop correctly assigned, are the

• Functions: while processing array elements, are all elements handled? Check the first and last element.

• Functions: when parsing a string, is also the last part of the string handled correctly? For instance a string is separated by ";"
  cStr := ABC;DEF;GHI
  WHILE AT(";",cStr) > 0
  ...ProcessPart(SUBSTR(cStr, 1, AT(";",cStr)-1)
  ...cStr := SUBSTR(cStr, AT(";",cStr)+1)
  ENDDO
  /* Now GHI is forgotten to be processed */

• Functions: in database searches, when the end-of-file is reached, is the last found record handled correctly?
• Variables: the maximum string length is 64K (65535 characters); is there any chance this limit is exceeded?
• Variables: using MEMOREAD is there a chance the file to be read is larger than 64K? If so, other functions should be used to read the file.
• Variables: the maximum array length is 4096. Is there any chance this limit is reached?
• Variables: the maximum numeric values to be expected, are they reached?
• Variables: for numeric database fields, is there a chance they are filled with a value larger than indicated? For instance, a numeric database field has length 2, then the number 100 will give a "DBFNTX/1021 Data width error".

Other considerations

Conditional compilation, e.g. #ifdef CORPORAT: Put #ifdef .. and #endif around each function. Functions will be moved or renamed, and the test for defines might be mixed up if during moving a function the tests are forgotten (it has happened before).

Index files should have the extension NTX (when using DBFNTX that is).

Seeking: DBSEEK(ALLTRIM(cTxt)) will also find cTxt+<any other text> if cTxt is not found.

General coding suggestions

This chapter contains miscellaneous considerations.

Memory usage

Since Clipper (in its 16-bit existence) has strong memory limitations (see heap and stack discussions on Phill Barnetts Oasis website and Wong’s articles on this subject), also in this project the use of variables should be limited. In short: for the heap and stack and Eval stack only 640K is available.

See:
http://www.the-oasis.net/clipper-8.html
http://www.the-oasis.net/clipper-5.html

Therefore STATIC variables must be replaced by STATIC array entries, for with the separate entries are used with a define. This reduces the chance of an Unrecoverable Clipper Internal Error: 650 Processor Stack Fault meaning the stack is exhausted. Also Eval errors occur less frequently.

Class oGet uses the heap; if the heap is set too low then if controls are created ON INIT then they are not shown. Therefor in our code the use of creating controls from ON INIT is reduced. Even then you might run out of heapspace.

Several other actions have been undertaken to minimize the number of variables. When writing new code you should keep in mind that adding new STATICs, PUBLICs will decrease the memory available, while PRIVATEs and LOCALs will reduce the stack.

It is best to have a function that operates on variables passed to it then have a function that operates on global variables. This reduces memory overhead (the global variables are kept in memory all the time while parameters are placed on the stack as long as the function is active) and clarifies the code.

Even replacing PUBLICs with STATICs that are referenced through functions is better: the scope is better defines, and you can use an array for a set of STATICs, which will reduce memory usage.

The Memory Low error is a result of conventional memory being exhausted. Unfortunately, there is no function (as far as I know) that can tell you are running out of conventional memory. The function that indicates the conventional memory does not report the complete memory pool used for conventional memory. A solution might be to link in a Virtual Memory object that comes with the latest Fivewin. I have tested it and it does not get rid of the memory low error.

Variables scope

Using functions inside ORDCREATE and ORDCONDSET building an index all parameters of the function must be known at all times. So either use constants or the MEMVAR type. If you miss this, you get an hangup in DBSETORDER or ChangeOrder with or without a Clipper Internal Error DBCLOSEALL(0). Apparently Clipper tries to reactivate the function.

Codeblocks cannot use local variables. E.g.:
EditPostVal(aFld, { |aGet| Chk(aGet, aArr[nT]) } )
does not work if nT is a variable: the last known value of nT is used at the time Chk is being executed.

Alternative: make the Chk function retrieve the value based on aGet:Name or aGet:VarGet() and use an array in which a coupling is stored between the name of the variable in aGet and the value of aArr[nT]. Or use an evaluation &("Chk("+aArr[nT]+")". Note: aGet cannot be passed as parameter because it doesn't exist within the evaluation.

The next construction gives errors:
A user selects from a number of possible date fields.
cDateField := aDateFields[1] /* 5 characters, while aDateFields[2] is 10 */
EditFldAdd ( ..., "cDateField", aDateFields, EDIT_COMBORIGHT )
After the user has made a choice cDateField is still maximal 5 characters, while it should have been 10, if choice 2 was selected.

Filters versus scope

In general, filters should not be used within the Clipper Fivewin environment. This is an extremely slow method. Better it is to:

• Set the order to the display order
• Seek to the first occurrence

For browses:

• Set the browse fields
• Set the scope
• Start browsing

For searching:

• Skip until the record contents does not longer match the requirements

This method is much faster than filtering, because you skip the item records that are not relevant. Using a filter will evaluate each record.

Use of SysRefresh

Be very careful on using SysRefresh. It gives Windows control over your application. Your program could be started in places where you don’t expect it. If no user action is permitted for instance while your code is updating a database, prevent any user action and don’t use SysRefresh(). If you don’t prevent it, you can get very funny effects.

Example 1:

For instance, the menu can be activated before closing a window is ready. This may lead to bugs like: "Error BASE/1132 Bound error: array access" (the browse handle has become invalid), or "Alias X does not exist" (since the code opening the address file has not been executed correctly).

Example 2:

For instance if SysRefresh would have been used in the WaitInfo function, it would make the dialog non-modal and the purpose the user has to wait for the file to exist but no longer than nSecs is gone.

/*=========================================================================*/
WaitInfo
/*=========================================================================*/
FUNCTION WaitInfo(oDlg, cFile, nEndSecs)
...LOCAL nCurrSecs
...LOCAL nOldSecs

...nCurrSecs := SECONDS()
...DO WHILE !FILE(cFile) .AND. (nEndSecs==0 .OR. nCurrSecs < nEndSecs)
....../* Don't use SysRefresh() here for then the dialog is no longer modal */
......CursorWait()
......nOldSecs := nCurrSecs
......nCurrSecs := SECONDS()
......IF INT(nOldSecs) != INT(nCurrSecs)
.........oDlg:cMsg := cFile+" "+NTRIM(INT(nEndSecs-nCurrSecs))
.........oDlg:Say( 1, 0, xPadC( oDlg:cMsg, oDlg:nRight - oDlg:nLeft ))
......ENDIF
...ENDDO
RETURN NIL

/*=========================================================================*/
WaitInfoUntilExist
Wait until given cFile exists for no longer than nSecs seconds.
If nSecs < 0 then only do SysRefresh; if equals 0 don't check seconds.
Returns whether the file is there.
/*=========================================================================*/
FUNCTION WaitInfoUntilExist(cFile, nSecs)
...LOCAL nStartSecs

...Monitor("> WaitInfoUntilExist "+IF(cFile!=NIL,cFile,"NIL")+" "+IF(nSecs!=NIL,NTRIM(nSecs),"NIL"))
...IF nSecs < 0
......SysRefresh()
...ELSE
......IF nSecs==0
.........nStartSecs := 0
......ELSE
.........nStartSecs := SECONDS()
......ENDIF
....../* Set applications window to front because else MsgRun takes WinWord */
....../* or whatever is front as current active window parent */
......SetActiveWindow(GetWndApp())
......MsgRun(STR_WAITING, cFile, {|oDlg| WaitInfo(oDlg, cFile, nStartSecs+nSecs)})
...ENDIF
...Monitor("< WaitInfoUntilExist")
RETURN FILE(cFile)

Maximum file size DBF and DBT

If you expect that you can reach the maximum number of records in the DBF you should check for it. I guess the maximum is about one million (Clipper 5.2).

The maximum size of a DBT is 32MB. When the users of the product intensively use the product, a check is mandatory. A check is provided in Chapter 9 Appendix: File functions.

File function

Because checking for a file using FOPEN can return an error, new file functions have been written to work around this. See Appendix: File functions.

Enforcing the coding standard

To enforce the coding standard several tools, and debugging options are available. Also doing the review using the code review checklist will have great benefits.

Tools

GROK

GROK can provide the following code checking capabilities (with respect to this coding standard):

Besides this, GROK has more handy features such as generating a function overview of a file, and displaying all defines and variables used for one source file.

See: www.prgtools.com, or contact John Kaster at Inprise.

Click! 2.03 Source Code Reformatter

To determine which functions are called by what and where, use this source code reformatter (well, it does more than that in fact).

For formatting Clipper source code, see: http://www.the-oasis.net/click00.htm

Debugging

In some cases CLD may be linked with the executable to debug the code.
In most of our projects we use debugging statements such as the one explained here.

A. Monitor statements without levels

These are displayed first and allow nesting and logging of seconds. See Appendix Debugging using Monitor statements without levels.

It allows display of seconds. This is required if you want to improve performance and need to know what the slow parts are. For example, RAID 5 systems are known to be slow in database transactions and virus scanners will also slow down creation of intermediate files like the ones needed on (re)indexing.

Also you can use nesting to be able to detect functions that go in too deep causing a Processor stack fault.

B. Monitor statements in levels of severity

As an alternative you can also separate the Monitor statements in levels of severity. These are explained in Appendix Debugging using Monitor statements with levels.

In both cases it is good practice to mention the incoming parameters in the first Monitor statement, and the outgoing parameters and return parameter in the last Monitor statement.

The incoming Monitor statement starts with "> " followed by the function name and the incoming parameters.

The outgoing Monitor statement starts with "< " followed by the function name and the outgoing parameters.

It is not necessary to mention the function name since the Monitor statement itself can print it. Whatever choice is made: as long as it is done consistently: always mention the function name or never mention it.

Compiler settings

To use the full capabilities in error detection the –es2 Clipper compiler flag should be used. Any error or warning should be taken care of.

How to review

I will shortly describe the procedure to follow for reviewing.

Preparation

• Decide who will be the moderator. This is the chairman who keeps the time schedule, prevents discussions longer than one minute, and makes sure that it is a team effort to increase code quality.

• Determine what to review. Limit the size of the code to review, especially the first time the code is reviewed.
• Hand out the things to review at least two days before the actual review is held.
• Limit the time to review. If a review takes more than one hour, schedule a new review meeting for the rest of the code review.
• Limit the number of persons doing the review; six persons is the maximum, two the minimum, but three is better.
• Don’t let managers attend a review, unless they are part of the development team or have reviewed themselves.
• The persons doing the review should have written down their remarks or have indicated the parts in the code they want to make a remark on. This is an indication the review is prepared and will lead to finding more defects in the code.

The documents or code to review should contain

• A clear description of the background (or a reference to such explanation).
• If it is the first review of code then the complete code to be reviewed, along with references to related code
• If it is the next review of code then the difference between old and new code should be shown, e.g. using a VSS print out or DIFF with the new code next to it.

During review

• Focus on major issues, errors in functionality, check the design with the code, bugs, rather than on layout style. If for instance indentation is incorrect, then just mention it once. If someone doesn’t understand a part of the code, then the explanation can be done in the reworked review list or preferably in the code itself, rather than spend time in the review meeting itself.
• Do not make personal remarks but positive critical remarks; purpose is code quality and learning of each other
• Write down all remarks (see the remark list template in Remark list template). Note that the remarks do not need to be typed in unless your handwriting is awful.

After review

• Take time for rework and have it planned in
• Rework all remarks, explain the questions, or give a reason why
• Once the rework is done, hand out the remark list with per remark a reaction being handled, answered or rejected with reason.

See also "The Seven Deadly Sins of Software Reviews" by Karl Wiegers:
http://msdn.microsoft.com/lubrary/periodic/period98/SDMag.htm

Code review checklist

Appendix Example

For instance:

STATIC FUNCTION PrintSel(cMode)
...LOCAL aFilter
...LOCAL cUserVal := ALLTRIM(Decrypt(USERS->LOGIN, ENCRYPT_KEY))

...IF cMode == "RA"
......aFilter := {"ADDRESS->EXISTING SELECTION"}
...ELSE
......IF cMode == "RP"
.........aFilter := {"DOCUMENT->EXISTING SELECTION"}
......ELSE
.........aFilter := {"FOLDER->EXISTING SELECTION"}
......ENDIF
...ENDIF
...RepAdmin(.F., aFilter,cUserVal)
RETURN NIL

Better:

/*=========================================================================*/
PrintSelDef
Print the selection definition.
/*=========================================================================*/
STATIC FUNCTION PrintSelDef(cMode)
...LOCAL aFilter
...LOCAL lOk
...Monitor("> PrintSelDef cMode ["+cMode+"]")
...lOk := .T.
...DO CASE
......CASE cMode == ADRSEL_SOORT
.........aFilter := {"ADDRESS->EXISTING SELECTION"}
......CASE cMode == POSTSEL_SOORT
.........aFilter := {"DOCUMENT->EXISTING SELECTION"}
......CASE cMode == DOSSEL_SOORT
.........aFilter := {"FOLDER->EXISTING SELECTION"}
......OTHERWISE
.........Monitor(" PrintSelDef mode unknown or not implemented "+UtoC(cMode))
.........lOk := .F.
...ENDCASE
...IF lOk
......RepAdmin(.F., aFilter, ALLTRIM(Decrypt(USERS->LOGIN, ENCRYPT_KEY)))
...ENDIF
...Monitor("< PrintSelDef "+LtoC(lOk))
RETURN lOk

Differences:

1. Header is available
2. Function name PrintSel resembles PrintSelection and works confusing, so I have named it PrintSelDef (an other name would be good as well, as long as the name covers the functionality of the function and does not lead to confusion with other function names).
3. Instead of IF .. THEN ... ELSE IF .. a CASE statement is used.
4. cMode is used as a define elsewhere in selekt.prg, never as "RA" or "RP". So instead of using "RA" and "RP", use the defines.
5. When using a CASE or IT THEN ELSE IF THEN ELSE statement, always use the OTHERWISE, or trap the unexpected ELSE. You might never know who will use your function and whether the call to your function is irratic.
6. If an unexpected error occurs, for instance as a result of a programming error, always Monitor the situation, because it will save time tracking the error.
7. Function begins and ends with Monitor statement. This is useful for tracking down errors.
8. Returns a value so that the error can be detected by the calling function.

Appendix Debugging using Monitor statements without levels

Usage:

#include "monitor.ch"
FUNCTION MyFunction(uParam)
...LOCAL lOk

...Monitor("> MyFunction ["+UtoC(uParam)+"]")
...lOk := .F.
...IF uParam != NIL
......lOk := .T.
......Monitor(" MyFunction setting lOk to True")
...ENDIF
...Monitor("< MyFunction "+LtoC(lOk))
RETURN lOk

Compile sources with –dMONITOR if you want to use the Monitor statements.

Monitor.ch:

#ifndef MONITOR
...#define Monitor(a)
#endif

Monitor.prg:

#ifdef MONITOR
...STATIC fDb := -1 /* debug file */
...STATIC nDebugVolgnr := 1
...STATIC nDebugMode := 1
...STATIC aFileHandles := {}
#endif

#ifdef MONITOR

/*===================================================================*/
GetDebugMode
nDebugMode := 0: don't output monitor statements
nDebugMode := 1: monitor statements as is
nDebugMode := 2: monitor statements with seconds
nDebugMode := 3: monitor statements with indentation
nDebugMode := 4: monitor statements with indentation and seconds
nDebugMode := 5: monitor statements with indentation and seconds
but without object functions containing ":"
Note: do not take monitor statements within this function!
/*===================================================================*/
FUNCTION GetDebugMode()
...LOCAL tObj
...LOCAL cIniPath
...LOCAL cDebugMode
...LOCAL nDebugMode

...cIniPath := GetWinpostIniPath()
...cDebugMode := ""
...tObj := tIni():New(cIniPath)
...tObj:Get(OPTION_SECTION, "DebugMode", "0", @cDebugMode)
...nDebugMode := VAL(cDebugMode)
...IF nDebugMode>5.OR.nDebugMode<0
......nDebugMode := 0
...ENDIF
RETURN nDebugMode

/*===================================================================*/
OpenDebugFile
Opens the debug file DEBUG<nmbr>, until a debug file is found,
nmbr <= 999 that does not exist and creates it. If all 999 files
exist then DEBUG000 is used and emptied.
/*===================================================================*/

STATIC FUNCTION OpenDebugFile(cTxt)
...LOCAL cExePath

...cExePath := GetExePath ()
...nDebugVolgnr := 1
...IF (fDb == -1)
......fDb := FOPEN(cExePath+"DEBUG"+ALLTRIM(STR(nDebugVolgnr)), 2)
....../* Open een debug file dat nog niet bestaat, max 999 debug files */
......WHILE (fDb != -1) .AND. nDebugVolgnr < 999
.........FCLOSE(fDb)
.........nDebugVolgnr++
.........fDb := FOPEN(cExePath+"DEBUG"+ALLTRIM(STR(nDebugVolgnr)), 2)
......END
......IF (fDb != -1)
.........FCLOSE(fDb)
.........nDebugVolgnr := 0
.........FERASE(cExePath+"DEBUG0")
......ENDIF
......fDb := FCREATE(cExePath+"DEBUG"+ALLTRIM(STR(nDebugVolgnr)), 0)
......IF fDb != -1
.........FWRITE(fDb,"DEBUG"+ALLTRIM(STR(nDebugVolgnr)) + " " +;
..................DTOC(DATE())+" "+TIME()+CHR(13)+CHR(10))
.........IF nDebugMode==2.OR.nDebugMode==4.OR.nDebugMode==5
............FWRITE(fDb, PADR(PROCNAME(2),15)+"("+STR(PROCLINE(2),5)+") "+STR(SECONDS())+;
........................" "+cTxt+CHR(13)+CHR(10))
.........ELSE
............FWRITE(fDb, PADR(PROCNAME(2),15)+"("+STR(PROCLINE(2),5)+") "+cTxt+CHR(13)+CHR(10))
.........ENDIF
.........FCLOSE(fDb)
......ENDIF
...ENDIF
RETURN fDb

/*===================================================================*/
Monitor
Writes text to the debug file.
If the debug file does not exist, the file is created.
/*===================================================================*/
FUNCTION Monitor(cTxt)
...LOCAL nNesting
...LOCAL cProcName
...LOCAL cExePath
...cExePath := GetExePath ()
...IF nDebugMode==NIL.OR.nDebugMode>0
......IF fDb == -1
........./* Track debug mode, from INI file: */
.........nDebugMode := GetDebugMode()
.........IF nDebugMode>0
............fDb := OpenDebugFile(cTxt)
.........ENDIF
......ELSE
.........fDb := FOPEN(cExePath+"DEBUG"+ALLTRIM(STR(nDebugVolgnr)), 1)
#ifdef MONITOR
.........IF fDb==-1
............SysRefresh()
............Error("Monitor could not open DEBUG"+ALLTRIM(STR(nDebugVolgnr))+"; Error "+NTRIM(FERROR()))
.........ENDIF
#endif
......ENDIF
...ENDIF
...IF nDebugMode>0
......IF fDb != -1
.........nNesting := 1
.........IF nDebugMode==3.OR.nDebugMode==4.OR.nDebugMode==5
............WHILE PADR(ALLTRIM(PROCNAME(nNesting)),30)!= PADR("MAIN",30).AND.nNesting<250
...............nNesting++
............ENDDO
.........ENDIF
.........FSEEK(fDb, 0, 2) // set file pointer at end of file
.........IF nDebugMode==2.OR.nDebugMode==4.OR.nDebugMode==5
............cProcName := ALLTRIM(PROCNAME(1))
............IF (nDebugMode==5.AND.AT(":",cProcName)==0).OR.nDebugMode!=5
...............FWRITE(fDb, PADR(PROCNAME(1),15)+"("+STR(PROCLINE(1),5)+")"+; .....................STR(SECONDS())+SPACE(nNesting)+cTxt+CHR(13)+CHR(10))
............ENDIF
.........ELSE
............FWRITE(fDb, PADR(PROCNAME(1),15)+"("+STR(PROCLINE(1),5)+")"+; ..................SPACE(nNesting)+cTxt+CHR(13)+CHR(10))
.........ENDIF
.........FCLOSE(fDb)
......ENDIF
...ENDIF
RETURN .T.

/*=========================================================================*/
MonitorDatabases
Monitor the state of all opened databases.
/*=========================================================================*/
FUNCTION MonitorDatabases()
...LOCAL i
...LOCAL j

...IF EMPTY( ALIAS(1) )
......Monitor("No databases in use")
...ELSE
......Monitor("Databases")
......FOR i := 1 TO 255
.........IF !EMPTY( ALIAS( i ) )
............Monitor(STR( i, 3 ) + ": "+IF(SELECT()== i, "=> ", " " )+ALIAS(i)+;
...................." current record "+NTRIM((ALIAS(i))->(RECNO()))+;
...................." relation nr "+NTRIM((ALIAS(i))->(DBRSELECT()))+;
...................." expr "+(ALIAS(i))->(DBRELATION()))
............FOR j = 1 TO 15
...............IF !EMPTY((ALIAS(i))->(INDEXKEY(j)))
..................Monitor(IF((ALIAS(i))->(INDEXORD())==j, "=> ", " " )+(ALIAS(i))->(INDEXKEY(j)))
...............ENDIF
............NEXT j
.........ENDIF
......NEXT i
...ENDIF
RETURN NIL

#endif

Appendix Debugging using Monitor statements with levels

Usage:

#define DBGMODE_ALL 3 /* Monitor all */
#define DBGMODE_FLOW 2 /* Monitor warnings, errors and flow */
#define DBGMODE_WARN 1 /* Monitor warnings and errors */
#define DBGMODE_ERR 0 /* Monitor errors */

FUNCTION MyFunction(uParam)
...LOCAL lOk

...Monitor(DBGMODE_FLOW, "> MyFunction ["+UtoC(uParam)+"]")
...lOk := .T.
...IF uParam == NIL
......lOk := .F.
......Monitor(DBGMODE_ERROR, " MyFunction Parameter must be filled")
...ENDIF
...Monitor(DBGMODE_FLOW, "< MyFunction "+LtoC(lOk))
RETURN lOk

/* Debug file related */
STATIC fDb := -1
STATIC nDebugVolgnr := 1
STATIC nDebugMode := DBGMODE_ERR

/*=================================*/
Monitor functions
/*=================================*/

/*==================================================================================*/
Set debug mode
/*==================================================================================*/

FUNCTION SetDebugMode(nNewDebugMode)
...nDebugMode := nNewDebugMode
RETURN .T.

/*==================================================================================*/
Get debug mode
/*==================================================================================*/
FUNCTION GetDebugMode()
RETURN nDebugMode

/*==================================================================================*/
Monitor(nDebugLevel,cTxt)
Calls for writing into debug file
/*==================================================================================*/
FUNCTION Monitor(nDebugLevel, cTxt)
...LOCAL cExePath
...LOCAL nNesting

...cExePath := GetExePath ()
.../* Check whether parameter list is correct */
...IF VALTYPE(nDebugLevel)=="C"
......MsgAlert("Error: Monitor called with wrong first param: "+UtoC(nDebugLevel))
......cTxt := UtoC(nDebugLevel)
......nDebugLevel := DBGMODE_FLOW
...ENDIF

.../* Create/open debug file */
...IF nDebugLevel <= nDebugMode
......IF (fDb==-1)
.........fDb := OpenDebugFile(cTxt)
......ELSE
.........fDb := FOPEN(cExePath+"DEBUG"+ALLTRIM(STR(nDebugVolgnr)), 1)
......ENDIF
......IF (fDb!=-1)
.........nNesting := 1
.........WHILE PADR(ALLTRIM(PROCNAME(nNesting)),30)!= PADR("MAIN",30).AND.nNesting<250
............nNesting++
.........ENDDO
.........FSEEK(fDb, 0, 2) // set file pointer at end of file
.........FWRITE(fDb, PADR(PROCNAME(1),10)+"("+STR(PROCLINE(1),4)+")"+SPACE(nNesting)+cTxt+CHR(13)+CHR(10)
.........FCLOSE(fDb)
......ENDIF
...ENDIF
RETURN .T.

/*===================================================================================*/
OpenDebugFile
This function writes the given string into a debug file
/*===================================================================================*/
STATIC FUNCTION OpenDebugFile(cTxt)
...LOCAL cExePath

...cExePath := GetExePath ()
...nDebugVolgnr := 1
...IF (fDb == -1)
......fDb := FOPEN(cExePath+"DEBUG"+ALLTRIM(STR(nDebugVolgnr)), 2)
....../* Open een debug file dat nog niet bestaat, max 999 debug files */
......WHILE (fDb != -1) .AND. nDebugVolgnr < 999
.........FCLOSE(fDb)
.........nDebugVolgnr++
.........fDb := FOPEN(cExePath+"DEBUG"+ALLTRIM(STR(nDebugVolgnr)), 2)
......END
......IF (fDb != -1)
.........FCLOSE(fDb)
.........nDebugVolgnr := 0
.........FERASE(cExePath+"DEBUG0")
......ENDIF
......fDb := FCREATE(cExePath+"DEBUG"+ALLTRIM(STR(nDebugVolgnr)), 0)
......IF fDb != -1
.........FWRITE(fDb,"DEBUG"+ALLTRIM(STR(nDebugVolgnr)) + " " +;
....................DTOC(DATE())+" "+TIME()+CHR(13)+CHR(10))
.........FWRITE(fDb, PADR(PROCNAME(2),10)+"("+STR(PROCLINE(1),4)+")"+cTxt+CHR(13)+CHR(10))
.........FCLOSE(fDb)
......ENDIF
...ENDIF
RETURN fDb

Appendix: File functions

IF <no other person is active>.AND.FileLength(cDBTFile) > 30000000
...<Indicate the user to take action to decrease the DBT file size, e.g. by copying the non-deleted records to a copy; note that packing does not work>
ENDIF

/*=========================================================================*/
FileLength
Use only when cFile is not already in use, this will cause serious shit
/*=========================================================================*/
FUNCTION FileLength(cFile)
...LOCAL nLength
...LOCAL nHandle

...Monitor("> FileLength "+cFile)
...nLength := -1
...IF FileReadable(cFile)
......// Open the file read-only
......IF (nHandle := FOPEN(cFile)) >= 0
.........// Get length of the file
.........nLength := FSEEK(nHandle, 0, FS_END)
.........// Reset file position to beginning of file
.........FSEEK(nHandle, 0)
.........FCLOSE(nHandle)
......ELSE
.........Monitor(" FileLength File open error:"+NTRIM(FERROR()))
......ENDIF
...ENDIF
...Monitor("< FileLength "+NTRIM(nLength))
RETURN nLength

/*=========================================================================*/
FileReadable
Check whether file exists and whether it can be opened for reading.
Note the difference with the FILE function: when Access is denied, FILE
considers the file existing, while FileReadable considers it wrong.
/*=========================================================================*/
FUNCTION FileReadable ( cFile )
...LOCAL lResult
...LOCAL fTarget

...Monitor("> FileReadable ["+cFile+"]")
...lResult := .F.
...fTarget := FOPEN(cFile, FO_READ+FO_SHARED)
...IF fTarget == -1
......lResult := .F.
...ELSE
......lResult := .T.
......FCLOSE(fTarget)
...ENDIF
...Monitor("< FileReadable "+LtoC(lResult))
RETURN lResult

/*=========================================================================*/
FILE
If file exist but cannot be read, it is considered to be available.
/*=========================================================================*/
FUNCTION FILE(cFile)
...LOCAL lResult
...LOCAL fTarget

...Monitor("> FILE ["+cFile+"]")
...lResult := .F.
...fTarget := FOPEN(cFile, FO_READ+FO_SHARED)
...IF fTarget == -1
......lResult := .F.
......IF FERROR() == 5 /* Access denied */
.........lResult := .T.
......ENDIF
......Monitor(" FILE Ferror: "+ALLTRIM(STR(FERROR())))
...ELSE
......lResult := .T.
......FCLOSE(fTarget)
...ENDIF
...Monitor("< FILE "+LtoC(lResult))
RETURN lResult

Remark list template

Review of: <file>/<function>

Author: <name of author>

Date of review: <date>

Moderator: <moderator>

Attending: <person 1>, <person 2>, …, <person n>

More information

Clipper

For links and other Clipper info:

See http://www.the-oasis.net

Fivewin

For Fivewin:

http://www.FiveWin.Be.kz
http://www.fivetech.com

The last contains access to the Fivewin newsgroup on the Fivewin server

news.fivetech.com: local.fivewin.english.

Other coding standards

See Coding standards for Java, C++, Delphi, Visual Basic and others.