Having finished the tidying of the
LCFG::Client::Build module I have now moved onto
LCFG::Client::Daemon. The first thing which caught my eye was the handling of the state tables. These state tables are used to control how the daemon handles notifications from the server, timeouts and signals from the LCFG client component. I pretty much totally rewrote the
MakeTable function so that it processed the input text and built the data structures for the tables in a much cleaner and more comprehensible manner. As with previous changes, my first step was to write some tests which checked the old function then ran them again with the new code to ensure I had not altered the API. I also introduced a new
NextStateInTable function which contained code which was previously duplicated inside
NextState. Finally I introduced an
InitStateTables function which is called from within
ServerLoop which hides the initialisation of the global variables used to hold the state tables. This means we now have a much cleaner API for handling all the state transitions based around smaller, testable functions.
Having finished the tidying of the
LCFG::Client::Build module is the largest part of the LCFG client code. It weighs in at 1800 lines which is nearly 50% of all the code in the project. It contains a lot of functionality related to processing the data from the XML profile into the format stored in the local DB file and triggering components to reconfigure as necessary. Improving this code was always going to be a big task but at least once this module is done the remainder will seem easy.
The main changes which stand out are, like with
LCFG::Client, related to noticing repeated coding of the same functionality. The first larger change came from noticing that in many places the value of an attribute (for example, the LCFG resource value) are decoded using the
HTML::Entities module but only for LCFG profile version 1.1 and newer. Now we probably haven’t supported anything older than this for a very long time but it occurred to me that rather than just drop the version check it would be better to completely enhance the attribute decoding. So, rather than have calls to
HTML::Entities::decode all over the place we now pass the value through a new
DecodeAttrValue function which in turn calls a new
NeedsDecode function to check if decoding is required. These are both small easily testable functions so I added a few tests along the way. The big benefit here is that if we now ever need to change the encoding/decoding of values and increment the profile version we are already prepared for the necessary code modifications.
The second big change was to improve the code of the
InstallDBM function. This had two copies of a complex regular expression used to parse a fully-qualified resource name (e.g. host.component.resource_name) so I moved this code into a new function named
ParseResourceName. Again this is now easily reusable and testable whereas before it was buried in the midst of other complex code. This led to some other improvements in how the debugging was done, I noticed there were many calls to
KeyType which was just returning a prettified name for the underlying attribute type indicators which are all single characters (in the set
[#%=^]). Each debug statement was very similar but handled a slightly different case, these were all merged into a
ResourceChangesDebug function. This new function massively improves code readability and also improves efficiency since it only actually does something when the "changes" debug option is enabled. By reworking the debugging it is now possible to use the
KeyType function in a totally generic manner. Anything which needs to know about the type of the attribute can work with the named versions rather than the almost-meaningless single character indicators.
There is still a lot more to do on this module to really improve the code standards but much of that might well be beyond the scope of this initial code cleanup project. The XML profile parsing and the DB handling are particularly in need of attention.