In the previous posts I reformatted the source code, had a first look at the code to see that it was compilable, had a look at how the data structure is organized in the directories, compiled the first executable writing or decompiling the missing functions.

The first executable, the main one, has the main menu but only a few of the functions are internal: in a lot of situations it just sets errorlevel and exits. Well, I discovered that it does this trick also when a workarea is chosen! You are presented with a list of workareas (client/year) on screen, select one and the program first sets the current work directory, then sets the errorlevel and finally exits: the batch file checks the errorlevel value and in this case restarts the main menu. But I want to eliminate the batch file, since I want to port all the code inside one executable…

Let’s start changing code. The first step is to define the strategy of the data dirs layout, something we already spoke about in
this post. There needs to be an environment variable that points to the root data dirs and all the subdirectories (one each for client/year) must be its children.
Current code uses a mix of curdir(), curdrive() and environment variable to determine the root data dir, then uses aDir to read the directory contents into an array (predefined with 512 items), loops on the array looking for a directory and when finds one, checks to see if there are 2 files and if both are found, opens one of them and reads a value that is added to an array. When the loop is completed, the maximum length of the strings in the array is calculated and finally the user is shown a form with an aChoice.

Would you write this code yourself, in this style? No, I won’t. And I’m trying to stay away for rewriting the whole function.

At first rewriting the whole function seems the smart thing to do. The code will look modern, in your personal coding style. But I don’t thing it will be a rewarding idea. This function seems to be a self contained one, with very weak relations with other part of the code. Infact it just sets a new working directory and exits! It will be used 5, 10 times per day, and if it is a few millisecond slower than the optimum it won’t be a problem. It’s a bit ’87 style, it uses aDir() instead of the more modern Directory(), it does some loop that is not necessary but the code works flawlessy, and it has been working for the last 20 years…

There are just a few things that I feel should be done and can be done quite safely. The programmer defined all the variables as PRIVATE. There are no LOCALs and no STATICs. This must raise a big alert flag for when I will put all the code in one executable. The programmer used a lot of macro expression: I can see a lot of them in the code, and they may refer to some of these PRIVATEs.
In this specific source file (but I think the situation is common) the PRIVATE variables are defined at the top of the file, in the first procedure (implicitely defined), also if they are used only in one function!
For a6/a7 variables a RELEASE statement let me understand that the variables are not used elsewhere, but just in that function. So I moved their definition from the first function to the function they are used:

PRIVATE a7[512],a6[512],nDir

becomes

FUNCTION ChooseWorkarea
LOCAL a7[512], a6[512], nDir

I could move some more variables from PRIVATE to LOCAL because a grep returned that the name was not used anywhere else, in none of the 200 source files. The program was going to exit anyway so probably it could be safe to LOCALize some more, but I want to be on the safe side at the moment. Refactoring is my passion, but I had to stop to continue the porting job!

The code is now modified as described in the previous post. The code that set the errorlevel was removed and a RETURN was added in the nidified code.

// set the working directory
CHDIR( drive + a8[ choice ] )
// FP: instead of setting errorlevel and exiting, return
RETURN

The code works. After the user choice a workarea, the control goes back to the main menu. I can confirm that the workarea is set because the function that handles the screen painter can find file1.dbf, open it and read the company name to display on-screen.