As described in the previous posts, a big number of duplicated functions had me change my mind on how to complete the porting. I’m now back to the original software setup: a batch file that calls several different executables depending on errorlevel values.

I reverted the changes done, starting from removing the source files from exe01.hbp and uncomment the initializazion function calls I disabled previously: the program compiled as described in the previous post (but I still have to solve that RETURN in the SEQUENCE)

Just to streamline the various .hbp files I’m going to create, I decided to have a global hbc files that lists all the use libraries. I could have used a hbmk.hbm file but I prefer to have everything listed.

I created libs.hbc file listing all the libs:

libpaths=.
libs=applib
libs=funcky
libs=mylib

In this way all the libraries don’t need to be listed in each .hbp but I just need to add a reference to libs.hbc. It is not only one line vs 4 in each hbp but something more strategic: in this way it is possible to add other libraries in a centralized place. For example, I’m thinking about the need to add some functions for converting the reports printed on file to PDF, and I may decide to use a library I already have: instead of updating all the .hbp files, I may just add the new one here. I also noticed that the error handlers I saw in the code are not called so I will probably need to add my error handler functions too.

I then created the file all.hbp with all the .hbp included but commented, except the one I already created.

-hbcontainer
applib.hbp
mylib.hbp
funcky.hbp
exe01.hbp
# exe02 is the Utility Menu, included in exe01
exe03.hbp 
# exe04.hbp
...
# exe13.hbp

Now with a single command I can compile everything in a standard, repeatable, way, all of them in one go, with the command:

hbmk2 -rebuildall all.hbp

Notice that the build will stop at the first error and you don’t have to check for errors like in a batch based solution. I think that all projects should have a all.hbp equivalent, a way to build the full project with one command, that can be used in an automatic building system.

I had the first 2 executables compiling, linking and running. The second has been patched but the patch not committed in the repository. I decided that before looking at that code (with the RETURN 300 lines after the BEGIN SEQUENCE and 600 before the END SEQUENCE) I’d better check if there are some other show-stopping problems. So I commented the exe03.hbp line in all.hbp and converted all .lnk files to .hbp, so that all executables are built except for the one I already know has a problem to solve.

Let’s list all the errors I found and how I solved them.

Error 1: RETURN inside BEGIN SEQUENCE/END. Harbour and Clipper 5.x syntax is probably different from Clipper 87: in the past it was possible to have a RETURN inside the sequence. This is probably the easier case as there isn’t a RECOVER and the return is only used at the start, if the files can’t be opened:

// original code
BEGIN SEQUENCE
   IF my_usedbf( .... )
      RETURN
   ENDIF
   ...

I changed to:

// changed code
IF my_usedbf( .... )
   RETURN
ENDIF
BEGIN SEQUENCE
   ...

Error 2: duplicated function names. This is a function clearly called by error handler. There are some of them in a file, but I never found the place where they are setup. I will check later. In this case I added a prefix.

Third program linked.

Error 3: missing function mkdir(). This function is used, in this program, to create a new workarea, and since a workarea is a subdirectory, it clearly needs to create the directory. It is also used elsewhere but before spending time developing and validating the function (with all that error handling the program uses) I’d like to complete the main port. So I created a stub mkdir() that outputs a trace and then exits the program.

Error 4: unexplicate error message by the linker: undefined reference to `HB_FUN(int0_t) static’.
Przemek explained why it happens and how can be solved. In this case the code uses a function named _size(). This function has a dbf file as parameter and it should return the file size. Since it is used together with mkdir() I created a similar stub file.

Fourth program compiled.

Error 5: undefined function, used by licensing code. In this program the licensing code is still active and this is strange: this makes me wonder if this executable is still used… I adapted the code as per other sources where licensing code is disabled.

Fifth and sixth programs compiled.

Error 5 again: another source code with licensing code active.

Several programs compiled.

Error 6: In a executable there are two missing functions reported by the linker. These functions have names that are already used in other parts of the code, in other executables. I checked if I correctly converted the .lnk to .hbp and I did. So, do I miss these functions? Are they duplicated but not present in the code I have? Should they be in the application library? or… or…
Both functions are used in a SET KEY statement and one has a source file with the same name, and the other is included in that file. I don’t have Clipper 87 installed but I feel there is something interesting possibly going on here. I’m going to ask in the newsgroup. In the meantime I’m commenting out the building of this executable.

Error 7: one comma not necessary in a list of variables

 PRIV var1, var2, var3,  // <- note the ending comma!

Executable compiled.

Error 8: 2 RETURNs in BEGIN SEQUENCE. In this case we have a 150 lines function that extracts data from several DBFs and depending on several conditions that show up during data processing, it RETURNs without completing, or BREAKs to skip some calculations but do something with the data found. It’s like having a 10 step process and after each step you may cancel (RETURN) or skip all the following steps (BREAK) and complete the job anyway. BREAK is used as an unconditional jump, without parameters, since it is used only to skip to the end of the sequence block.
My solution is the following: all original BREAKs, the ones that want the program flow to continue are tranformed to BREAK 1, while all RETURNs are transformed to BREAK 2 and finally a RECOVER block is added to properly handle the situation:

BEGIN SEQUENCE
...
IF condition()
//   BREAK
   BREAK 1
ENDIF
...
IF condition()
   // RETURN
   BREAK 2
ENDIF
...
RECOVER USING nValue
   IF nValue == 2 
      RETURN
   ENDIF
END SEQUENCE

And now also this last executable compiled.

Errors 1,2,5,7 and 8 were solved. Errors 3 and 4 are just stubs functions like the others working on directories. For error 6 I need infos from someone with a working Clipper 87. Of course there is still the RETURN in that big function to fully analyze, with nested SEQUENCE blocks, that probably will be solved as per error 8. Unfortunately, this block is in the most important executable, the one used to load the data inside the program… I will do in the next post.

I need to show the accountant lady that the program can be converted and the first round is complete: it works somehow, for sure it needs testing, but I think that there will not be too many problems in the future.

This is the title of chapter 11 from the book “The mythical man-month”, by Frederick Brooks. It is not exactly my case but I always have in mind the concept: you should be ready to change your mind, throw away the job done, and rollback or start from scratch.

You can do interview to users, analyze processes, write stories, you never know how a project will develop. In this case I’m doing a porting of a program written in the late 80s so I need to understand the original programmer workflow. My idea on how to port the program had to be validated spending as little time as possible. Some were succesful, now I’m facing the first one that didn’t, and it is a show-stopping one.

I repeated my idea several times: unify all the executables into one. The idea got a stop when I discovered, merging the first external group of source files, that the original programmer used function with identical names in different sources and that the code inside the functions is different!

Let’s have a look at the lockok() function:

FUNCTION lockok()

   SELECT dbf02
   IF ! my_file_lock( ...  )
      RETURN .F.
   ENDIF

   SELECT dbf05
   IF ! my_file_lock( ...  )
      RETURN .F.
   ENDIF

   ...

   RETURN .T.

It is a really simple code: it tries to put a file level lock on several files. This function is defined in 3 different source files (linked in 3 different executables, of course) with a different list of databases. It is used in places where the programmer wanted to be sure that nobody else could change the data during an update that spans 3 to 8 databases. Unfortunately, if the function reports that non all files could be locked, the program just skips the big update…

We have two easy ways to handle this situation. Both need that you understand to which executable belong the source files that have the function definitions and, more important, the ones where the function is just used. Once you trace them, you can simply prefix the function name with a identifier and change all the calls that belong to that executable, or refactor the function to receive the databases alias as a parameter.

In this case the second option is probably better, so that similar looking code is deduplicated. I also discovered that there are a couple of other functions, with similar sounding names, that perform the same job: probably the programmer was creating duplicates in his own programs..

This is a really simple case, one that can be handled very quickly. But the second is really more delicate. Infact the second duplicate function I analyzed was about 150 lines long and it looked really similar, I’d say that they were perfectly identical… I was about to port the function to the application library but in the end decided to use a diff tool and I finally noticed that a couple of lines were a bit different: 2 out of 150 lines were different in 2 IFs that had 2 expressions vs 3 expressions. I don’t know what the code really does, and if this check is really important to the data I’ll use and I don’t know if I could add it to the missing one. Solution: add a prefix to the function names.

In the previous post I already reported about one function duplicated, different, but used only in the source file they were defined: adding STATIC solved this case.

Another case was a function whose code is completely different but functionally equivalent. Imagine you have to report the sum of the values in a array and use a FOR loop once and a aEval() the other time. But to deduplicate the code I should write test cases to see that there are not edge cases handled differently.

Too many different cases: first 4 checked, 4 different problems and 4 different solutions. And there are 47 to go! Probably it’s time to change strategy. Merging this code I had a couple of compilation problems I solved with a temporary patch: disabling that code! So I decided to step back and fully mimic the original program, compile all different source code trying to clean up compilation errors and in meanwhile try to experiment a robust solution for passing the subdir to use as current working directory.

So I decided to throw away the changes done to deduplicate the functions and to start in a new direction.

I should feel more relaxed now that I demonstrated that the program builds, can run and, best of all, can print. But I’m not.

The original program had 16 executables. One is the main menu that I already compiled (but not fully tested), one is the Utility menu, that was merged into the main menu, but only for a couple of operations.

Now I think I face the biggest problem: one by one merging all the other executables source code. Now that the basic functions are working I think that it is better to work on the biggest external program, the one the accountant spend the most part of their day: the form to load data in the db.

This form is composed of several sub-windows, each with its own way of work, several function keys always active, tons of lookup tables, popup windows… it is a real pleasure look at the lady loading data typing on the keyboard at light speed, TABs, cursor keys, function keys: she knows the exact sequence to type.

I decided to split the job in two parts. In the first I will compile the program as is, like an external program, just to see if it compiles and, expecially, links. The second is to merge the code into the main one.

As done before, I hand converted the blinker style .lnk file to Harbour .hbp file. I then added the libraries and tried to compile. There was one error in compiling, a RETURN statement nested in a BEGIN SEQUENCE that I remmed just for testing, and several missing functions. I counted 7 missing functions and looking at how they were used, some of them looked complex. Time to decompile again. I was able to recover all of them and I added it to the applib.hbp file. I alse recovered a couple of functions I had to rewrite from the main menu executable.

I had to iterate the process twice since the recovered functions called other functions I hadn’t recovered yet. Finally both main executable and this new one compiled and linked without errors.

Testing the new executable is very easy: it just needs to be run with its current directory set to the client/year subdirectory. Simple. The program started with no problem and it seems to work: I can input data, lookup table appears, all that strange popup just keep popping up but I don’t know what to write in there. Just one note: it seems that some popups doesn’t restore the underlying screen when they close. I know that some windows related functions are just stubs, empty functions with no code. I will check that later.

One more thing to note is the RETURN inside a BEGIN SEQUENCE. I don’t know why it was allowed in Clipper 87 and it is no more. What makes me feel a bit worried is that BEGIN SEQUENCE is on line 300, the RETURN statement on line 604, the END SEQUENCE on line 1568! With a nested BEGIN/END SEQUENCE and several BREAKs. It will be a long night the one I will work on this.

Terminated succesfully the first part, I started the second, merging the code. Well, actually I added the new hbp files to the old. And compiled forcing a full rebuild.

I got 51 (FIFTYONE) duplicate functions! Until now I was fighting for missing functions. Now I need to fight with duplicate functions!

I don’t know why there is this situation in the code. But as I already said, the application library is without sources, and it is the only lib. Probably the programmer didn’t know how to create a lib. Or had some other obscure need to duplicate them.

These errors messages are like an alarm ringing in my head: why so much duplicates ? Are I really sure these are real duplicates?

Let’s check.

The first duplicated function reported is lockok(). I open the 2 source files and discover immediatle that no, they are not equal. Worst, a grep reveals that this function is defined in THREE different source files, and used in 7!

The second duplicated function is a callback function of a browse. Each one is defined and used in the same source code, so I could simply add a STATIC in front of them. In no other source file there is their name.

But for the first one, what to do? The only “easy” solution is to change the functions name with a prefix of the executable they were from, info that I need to extract from the .lnk files. As I said the function is defined in 3 files and this means there are 3 executables involved. So, it is easy to change the name of the functions defined and used in the same source, but I have to search for the other 4, which executable they are linked in.

This is the most important program and I have to solve 51 duplicates. In the process I need to solve more, like the first one that I saw is present in 3 files. But what will I find next, working on the other executables? Will I find dozens more duplicates?

Should I keep the executables distinct and find a way to set the working directory? It can be as easy as adding a memowrit() to save the working subdirectory and a memoread() to read it back…

I have to think about it.

As I said already, I like refactoring code. It’s an activity that is avoided by a lot of programmers. The first excuse is that it usually takes time, a lot of time. The second is that code works, it just works so no changes are needed and it is possible to introduce new bugs refactoring. The third is that there will be no real advantage to streamline the code.

I don’t want to do a post on why refactoring is important but I just want to reply to the 3 most common excuses:
1) refactoring should be done during code development and not when the product is completed
2) tests should be in place to verify how the code works before and after the refactoring
3) it really really depends. After a refactoring, code can be more clean and easy to read. Snippets of code repeated in various parts can be refactored to a function and be present only once. Refactoring is not specific to have quicker code but it is possible, as a side effect or when set as a goal.

In the previous post we had a look at a function that was coded, well, not properly.

FUNCTION record_lock( ..., wait, ... )
   PRIVATE row,col,time,video

   row = Row()
   col = Col()
   time = wait
   video = SaveScreen( 21, 0, 23, 79 )
   IF RLock()
      csrput( row, col )
      RETURN .T.
   ENDIF
   DO WHILE .T. // Loop for RLock() retries

Also the code in the loop needs some refactoring, but it is rarely used so it’s not a problem. Also the code shown here is not really a problem, since it correctly works, but when you call this function a lot of times, there is some overhead that we may avoid.

My refactoring:

FUNCTION record_lock( ..., wait, ... )
   LOCAL row,col,time,video

   IF RLock()
      RETURN .T.
   ENDIF
   row = Row()
   col = Col()
   time = wait
   video = SaveScreen( 21, 0, 23, 79 )
   DO WHILE .T. // Loop for RLock() retries

I timed the printing procedure I used as a test in the previous post, where the function is called 15600 times, before and after the code changes. From 4.85 to 4.15 seconds (timing averaged on several tests). It’s just 0.7 seconds shaved, but it’s about 14.5% quicker now. For a 2 minutes work.

I wanted to perform another test: opening the files in exclusive mode. I changed the flag used to open the database files and removed the test in the printing loop.
The average time dropped to 3.32 seconds, a saving of 31.5%
Just for a couple of changes!

I did measure before doing any changes: after the first and simplest ones I got a 14.5% speed increment. Opening the files in exclusive mode I saved 31.5% of the time but I now have to change code elsewhere because now if another instance of the program is run, it won’t work properly. Infact I opened all the files in exclusive mode but instead I should open a couple of them in exclusive and the others in shared mode. This means adding lines, keep them in order as the program expect them, fully test that the program doesn’t break if a file is found opened by another instance of the program…. and unfortunately it breaks.

The code is almost all multi-user ready (files opened in shared mode, records locked, etc) but the programmer that sent the code to me said that it shouldn’t be used in multi-user. Don’t know why, anyway these last changes break the program.

Revert the last change, back to shared mode.

I imagine it wouldn’t be a problem to wait 4.15 seconds instead of 3.5 seconds when this job took hours.

PS: it’s really difficult timing code snippets on a multi tasking operating system working on a multi core cpu, when you can’t control exactly what other programs the pc is running in that moment. Anyway, I removed the GOTO RecNo() line and I saved another 0.40 seconds.