The first round of tidying code to satisfy perlcritic was focussed on the central LCFG::Client module which contains code used by all the other modules.
As well as the tidying there were a couple of slightly larger changes. I had spotted that several routines (RPMFile, DBMFile and ProfileFile) were each doing their own mangling of the host FQDN and then doing similar work based on the results. To reduce duplication I introduced a SplitFQDN function which contains an improved version of the hostname splitting functionality (and which can now be used in other places). I then also introduced another new function (named HostFile) which contains all the other duplicated functionality between the 3 functions. Each of the 3 functions are now pretty much reduced to a single line call to HostFile with the relevant parameters set. At the same time as adding these new functions I added tests for them and also the higher-level functions which use them. As each has now been reduced in complexity it is much easier to test them. This gives me a good guarantee that if I have to make changes in the future they will continue to work as expected.
Beyond tidying the code to resolve the worst of the perlcritic issues I also applied a number of changes which come from the lower levels. In particular I removed a lot of unnecessary brackets associated with calls to built-in functions and conditionals. This might seem like a tiny thing but it does reduce the “noise” quite considerably and vastly improves the code readability. I also removed all uses of the unless conditional, this is something which drives me crazy, anything more than an utterly simple condition is very hard to comprehend when used in conjunction with unless. That is one feature I really wish was not in Perl! I’ve seen unless-conditions which are so complicated that only a truth table can fathom out what is going on…
Another code smell which was eradicated was the heavy usage of the default scalar variable ($_). In my opinion there is no place for using this in large code bases outside of situations like code blocks for map and grep. Using it pretty much guarantees that there will be the potential for weird, inexplicable action-at-a-distance side-effects in your code.
One thing I would like to spend more time on at some point is improving the naming of variables. There is frequent use of single-letter variable names ($c, $t, etc) which is mostly meaningless. This might not be a problem in a very short (couple of lines) block where the context is clear but in chunks longer than a screen-full it’s really hard to track the purpose of all the variables. There is also quite regular reuse of variable names within a subroutine which again makes mentally tracking the purpose of each variable very difficult.
Posted by squinney