The following is a suitably anonymised report about some source code which recently passed before me. You just can’t make this stuff up, some people really don’t know how to produce good code…


PROJECT Software Review

The following is the result of an extensive review of the source code for the PROJECT project supplied by CUSTOMER to REVIEWER. The delivered source code (codebase) was supplied as a single tar file, with some accompanying documentation as provided by the company subcontracted by CUSTOMER (the contractor) to implement the software requirements of the PROJECT project.

General observations about the delivered codebase

The codebase was supplied as an uncompressed POSIX tar archive rather than as a source code revisioning system store as would be expected of such a delivery. Therefore there is no way to track the history of the project, nor indeed any way to accurately suggest how much time was spent in developing the codebase.

Initial investigation suggested that the codebase should be unpacked in the directory “/opt/PROJECT”. Imposing such a requirement on the customer is extremely unprofessional. However to require that the codebase be unpacked in a directory in the “/opt” heirarchy, which is explicitly not a suitable location for source code as per the UNIX file system hierarchy standard, is utterly unacceptable.

Once unpacked, a preliminary audit of the codebase revealed several intermediate object files inside the source tree and also build system intermediates such as preconfigured makefiles. This indicates that the codebase was not cleaned (mechanically or manually) by the contractor prior to delivery.

The application appears to expect to be run as the super-user which is inadvisable within a UNIX environment. The build system itself expects to be run as the super-user as it implicitly overwrites configuration files belonging to the user called ‘root’. This is considered to be extremely poor practice and is strongly discouraged in relevant documentation regarding the building of software in a UNIX environment.

The supplied build script configures the system so that the machine will reset its database on every startup. This means that the PROJECT will not retain CRITICAL_DATA across reboots, power loss, or even restarts of the windowing environment.

The supplied internationalisation is a gesture rather than the pervasive presence which would be expected of such an effort. Indeed all the internationalisation appears to be is that which the “GNU autotools” provide by default as infrastructure for the developers to build around.

Given that almost every file in the codebase is marked executable, it is reasonable to assume that the codebase has passed through a Windows-based computer (or at minimum a poorly configured Linux system with Windows file systems) and this makes it very confusing when looking for scripts or tools during a code audit as everything is considered to be a program by a Linux system.

The codebase is a patchwork of different styles, indicating that the contractor employs several different programmers who do not adequately communicate with each other with respect to code structure and styling. This suggests that communication within the contractor with respect to design and requirements may also be poor.

The codebase appears to contain source code licensed under the “GNU General Public License” (GPL). The GPL is an open-source licence which does not permit direct linking or inclusion without all parts of the project being placed under the same licence. As a result, CUSTOMER would have to release the entire application under the “GNU General Public License” which requires that the authors supply the source code to anyone to whom they provide object code; or else risk a lawsuit from anyone who discovered such a discrepancy.

The fact that the contractor felt it acceptable to include source code encumbered by such a licence is not heartening; indeed it raises the question of the provenance of the rest of the code base as it is now not beyond reasonable doubt that, given the apparent patchwork nature of the codebase, the contractor may have included code wholesale from other projects they have been, or are, involved with.

General design structure and source code quality

It was extremely difficult to discern any form of overarching design from the codebase; raising concerns that there may have been insufficient time invested by the contractor in planning the software project.

Several files implemented the same “gross hack”, regarding the PROPERTY of the HARDWARE_ELEMENT, independently yet often in an identical manner. This single issue is indicative of a lack of understanding of the concept of correct code reuse within the project.

The codebase appears to implement exactly the use cases provided by CUSTOMER to the contractor. However this appears to have been done by layering hack upon hack until the application behaves as required, rather than (where appropriate) performing a fundamental refactor of the approach being taken with respect to a design.

The codebase required simple (but numerous) changes to syntax in order to be compiled. This suggests that the contractor has never compiled this codebase from scratch and thus is not regularly auditing its output.
Some of the coding styles in use within the codebase seem to call for mechanically extractable code comments which describe the interfaces and mechanisms within the software. This is a laudable practice which in this codebase has been thwarted by three major issues:
1.The code commentary is not pervasive.
2.The comments are often empty, misspelled or incomplete.
3.Several comments in a random selection checked were outdated, incorrect or erroneous.

A cursory inspection of a random selection of files uncovered an appalling lack of attention to spelling which has been propogated through the use of auto-complete features in editors and never spotted. This suggests that the contractor may not have performed sufficient internal code reviews during the development of the software.

The codebase has the feel of several independently constructed sections glued together by an integrator. This is a common practice within large software projects but relies on a very high degree of care and attention on the part of the integration lead. There is no evidence of the required level of competency in the delivered codebase.

The approach taken in developing the codebase has resulted in what is essentially little more than a set of cue cards with pictures on, backed by a small amount of inconsistent, and at times incorrect, logic. This shows a true lack of foresight in the design of the project; a lack of understanding that a piece of software which has a user interface is usually more than just a user interface; and a lack of sensitivity to the future requirements likely to come out of CUSTOMER.

The interface to the hardware, which is clearly one of the most important parts of this software project as the software is intended to drive a machine, appears to be scattered, incomplete and does not use the source code supplied by CUSTOMER to the contractor for this specific purpose. Otherwise it is so well hidden in the rest of the codebase that REVIEWER were unable to find it. Either way, this does not inspire confidence in the codebase.

While the presence of a database in the software’s architecture is commendable, the choice of SQLite was short-sighted and clearly did not take into account the resources available on the target system.

The supplied initial database, as found in the codebase, is a single binary lump. This means that were the codebase to be placed under a revision control system, it would be impossible to request the difference between versions, in the accepted manner, directly from the revision control system.

To have constructed this database from source during the build would have been trivial. That the contractor did not do this indicates that they exercised little or no change management during the production of the software.

The presence of the ‘Patch’ file containing the number of the PROPERTY is a direct example of how poor some of the source code design (and implementation) is.

The lack of care and attention used when selecting fonts for display, such that labels often spilled out of their display element (or in some cases directly overlapped neighbouring elements) indicates that the contractor’s developers did not understand how to correctly construct a user interface for today’s modern operating systems.

The combination of hand-drawn and software-produced user interface widgets within the same window indicates a lack of professionalism with respect to the look and feel of the application.

The build system for the software is incomplete and misuses the development tools it directly relies upon. It has hard-coded paths for its dependencies, instead of using the commonly available and accepted industry standard mechanisms of discovery, which results in a lack of portability to a build environment other than the contractor’s own.

Concerns regarding the supplied documentation.

The primary concern regarding the documentation is the clear lack of any substantive documents regarding the codebase itself. This would not be so much of an issue were it not for the inconsistent, appallingly badly written and in places incorrect, in-line code documentation.

The document entitled ‘Scope of the Release’ supplied by the contractor appears to simply be a list of use cases which the contractor had considered for the release. Several of them are marked as unavailable or incomplete for the release made for the TRADE_SHOW and many of them are marked as incomplete or unimplemented regardless. However, no summary was provided meaning that in order to discover the lack of useful content in this document, all forty-five pages needed to be read carefully.

Conclusion

In summary, the basic audit performed by REVIEWER has uncovered numerous points of concern within the codebase that any reasonable developer should have discovered.

That the codebase was delivered in this state is, simply put, incredible, worrying and disappointing. The quality of the codebase is such that in the opinion of REVIEWER, no professional contract developer would have delivered it as anything other than a throw-away proof of concept with an explicit note to the effect that the codebase itself contains no recoverable or re-usable source code.

It is the recommendation of REVIEWER that CUSTOMER discontinue the use of this codebase pending the acceptable resolution of all concerns raised in this document. It is also the opinion of REVIEWER that such a resolution would require a greater investment of time and money than starting a new implementation.

CUSTOMER may be able to recover some user-interface concepts and graphic elements from the codebase, however REVIEWER do not recommend the re-use of the source code or design itself.

Comments on this page are closed.