I just spent a day refactoring a service application written in Delphi 5. The original plan was to simply port it to Delphi 7 - which would involve switching from using the TNMFTP component to the Indy equivalent. That part of the job didn't take long, but I also wanted to try to fix some odd behaviours that the service exhibited. For example, it appeared to sometimes wipe out the data in the table it was supposed to fill with data, without warning.
In order to better understand the code, I went through a number of refactoring steps.
A symmetry between the acquisition and the release of resources is one of those things that really help make code bug resistant. In place of the word symmetry, you could also call it predictable release of resources. For lightweight resources (e.g. TStringLists) that must have a lifetime beyond a single routine (where a simple Create/try/finally/Free/end pattern would work), why not just allocate them in the constructor of the owning object, and free them in the destructor (or the respective events of a form or data module.)
Error handling code is often overlooked in testing, especially when programmers do their own testing. Looking over the error handling, I found something similar to this:
try
...
except
on E: Exception do
Exception.Create('Operation Failed:', E.Message);
end;
I'm pretty sure this code wasn't ever tested. How about getting rid of the whole try/except block?
I'm a big fan of debugging the simplest possible way. That means debugging a regular exe file, rather than try to be fancy and attach the debugger to a host executable that loads up my code. So I created a data module to separate the business processing out of the TService data module. I also introduced an include file with a $DEFINE to indicate whether to build the executable as a service or as a regular application. With a decent separation of service-management code and business logic, there were only a handful of places where I needed to use conditional compilation. As an added bonus, the application is now more modular, and can now run as a regular application, without being installed as a service.
Duplicate code is one of my favourite things to get rid of. I love to take any sets of 2 or more lines of code that look almost the same and turn them into a shared routine. Sometimes that might require that I create a new shared unit, and I honestly think that some programmers just copy and paste because they are too lazy to add a new unit.
The larger the scope of a variable is, the more difficult it is to ensure that it is only being used when it contains the expected value. Class members variables can often be reduced to local variables and/or method parameters.
Accurate variable and method names are very important. If a boolean variable is called DoneDidIt, what do you think that means? What if it was called ScheduledProcessCompleted instead? This goes hand in hand with the idea that a method should do exactly what is says it does, no more, no less. If a method is called LoadSettings, it should load the settings. Should it open a database connection? No.
Figuring out the simplest way to design an application in a modular way where a picture of all the dependencies point in the same direction (higher-level code depends on lower-level code, and never the other way around), is an important challenge early on in a development project. Properly partitioned code is much easier to maintain for several reasons.
By following the above ideas, you can read the code at any level and not need to know what is going on at the levels below the one you're at in order to find the place where you need to fix that bug, or add that new functionality.
Comments? As long as the code is self-describing, as simple as possible, and contains no technical workarounds, they aren't really necessary. Sometimes however it is easier to read a quick explanation than read through several lines of code, but there is always the danger of comments getting out of sync with the code, and no amount of testing will verify that it doesn't happen.
