3.3. Tidying the Code

The code itself was in a messy state when I took over, the indentation and naming schemes were inconsistent, having been worked on by several different people. I used a programme called astyle [astyle] to create a constant indentation and brackets policy. I chose to use tabs for all indentation, which is equivalent to 8 spaces. This large indentation makes it instantly clear where blocks begin and end. It also shows up areas with too many nested blocks, an indication of poor design.

Some attempt had been made in the code to use Hungarian notation naming. This is a syntax where variables and classes are prefixed with an indication of their type. So m_bIsInstance is a boolean member variable and CAssociationWidget is a class. There is a balance to be made between informative use of Hungarian notation and its distractive value. I removed the occasional uses of C to prefix a class name; it is obvious to the programmer what is a class especially since class names always begin with an upper case letter. I kept the Hungarian notation for member variables, which are declared far away in the code from where they are used, but removed it for method variables where the declaration should be no more than a few lines above its use.

Several enums are used in the code to keep track of widget and object types, and these were stored in a class called simply Uml. Namespaces are a relatively new feature to C++ and traditionally are not supported by all compilers, however I found no problems and received no complaints on replacing the class with a namespace. The lack of use of namespaces in C++ seems to come from a reliance on the kludged solutions used prior to their introduction than on any real lack of support for them.

There are bad practices throughout the code which I did not change because it would have taken too much time. Many methods use variables with single character names, or undescriptive names like temp. The -> dereference operator is often surrounded by spaces, despite being a very tightly coupled operator. C++ allows for method code to be included in header files rather than in the normal code files. This is supposed to help with optimisation but I find the feature frustrating because it makes methods hard to find and requires a complete recompilation of the programme when there are any changes to the methods. I removed some of the worst offending inline methods, those which contain more than trivial code.

I never did create a formal style document dictating guidelines for the layout of code. People's preferences vary and it is better to let them code in the style that they find easiest and most efficient to working. Almost all submissions though have followed my preferred style since the advantages of a constant code layout are obvious to anyone who has worked on code with other developers. However I have received complaints that the problems listed above and the lack of a formal coding style make the code base harder to get to grips with. Given that, and the amount of time I spend during coding sessions reformatting code to remove the problems listed above and make it more readable, I wish I had spent more time initially on this apparently superficial problem.

Flex is a computer language lexical analyser which is used by Umbrello to create the code importer. I was receiving a number of compilation problems from users who, it turned out, did not have Flex installed. By adding a further check in the autoconf files machines without Flex will now give out a helpful error message.

I received a patch for cleaning up the #include header file lines: removing headers which had been required but were no longer needed by the file and adding ones which were required but brought in by other includes (something which should not be relied upon). Unfortunately this patch removed a number of headers which were required for the files. Although it worked fine for the submitter it would no longer compile on my setup which shows the importance of testing configuration changes on a number of different systems. The only parts of this patch which I chose to accept were the replacements of a number of C includes replaced with C++ equivalents.

The compiler outputs warnings for several potential problems such as unused variables and implicit casting. In the case of implicit casting the required fix is usually just to add an explicit cast. Unused variables are a common occurrence with overridden methods, it is possible to just comment out the variable name in the method header to prevent a compiler warning. While most compiler warnings are unlikely ever to be issues which could cause a problem, it is good practice to remove any that occur so I spent some time going through the code to remove all the warnings.