Salvatore Iovene

Here we continue with explaining some of the mistakes commonly made in Object Oriented design, and the good practices that are often ignored. This article is focused on code maintainability and on improving cooperation with people working at the same project.

Encouraging class dependencies.

Having a lot of (mutual) dependencies in the code is quite typical of Spaghetti Code, and it’s definitely something we want to avoid, in order to keep our design neat, improve maintainability and ensure ease of collaboration with colleagues. What do I mean by “class dependencies”? Let’s continue the example from the last article, and suppose we have a certain class GuiManager which, at some points, wants to generate some reports. Let’s introduce now a certain ReportManager, which is a class responsible for generating reports. We have two types of report: TableReport, and ChartReport. They look like this:

class TableReport {
    public:
        void report()  {
            // do something
        }
};

class ChartReport {
    public:
        void report()  {
            // do something
        }
};

This means that the ReportManager will have to look something like this:

class ReportManager {
    public:
        void reportAll() {
            m_tableReport.report();
            m_chartReport.report();
        }

    private:
        TableReport m_tableReport;
        ChartReport m_chartReport;
};

There are several problems in this implementation. First of all, If the guy responsible for the TableReport one day wakes up, and decides that the method report() should rather be named generate(), he will not only be allowed to just change that and commit to the repository, but this will break the ReportManager! So after a few hours, the guy responsible for the ReportManager checks out from the repository, builds, and finds out that all the times he has used the TableReport need to be changed. Of course this is something we don’t want to happen.

The usual approach to this, is using an Abstract Base Class (ABC), which is a very robust way to sort out problems like this. Let’s see come code:

class Report {
    public:
        virtual void report() = 0;
};

class TableReport : public Report {
    public:
        void report()  {
            // do something
        }
};

class ChartReport : public Report {
    public:
        void report()  {
            // do something
        }
};

Report is our ABC, and with it we are literally forcing the people who write TableReport and ChartReport to write a method named report(). So, this way we broke one dependency: the ReportManager doesn’t need to worry about the way every single report will call the method: it’s sure that a method named report() will exist.

There is, tho, another dependency. If somebody writes a new report, say XmlReport, this will need modifications to the ReportManager, because our logic so far implies that the ReportManager knows about all the reports. So, if we’re not the maintainers of the ReportManager (because maybe it’s in some different library, written by someone else, and we don’t have access to the code), we will have to go ahead and ask the rightful maintainer to modify the code. Hence, there’s an extra dependency, not structural, this time, but logical. What if the maintainer of the ReportManager gave us tools (read APIs) so that we can register our particular report to the ReportManager? Consider the following code:

class ReportManager {
    public:
        void registerReport(Report const & r) {
            m_reports.push_back(r);
        }

        void reportAll() {
            std::list::const_iterator iter;
            for(iter = m_reports.begin();
                 iter != m_reports.end();
                 ++iter)
            {
                iter->report();
            }
        }

        private:
            std::list m_reports;
};

This way, the ReportManager doesn’t have to know anything about any Report.

Today I want to talk about Object Oriented practices, and 3 commonly made mistakes. Very often, when reviewing code written by other people, I find violations of common OO practices, that make the code a lot less maintainable. Here follows a list of the most common ones, and, of course, some explanations about them.

Layer violation.

While not the most common, this appears to me as the most dangerous. What is layer violation? Let’s show it with an example. Assume we have a GUI driven application that reads data from a database and shows the results on the display. We might consider having some upper level Controller class, and managers for each component, e.g. GuiManager, DbManager, ReportManager. Assume that the Controller class runs a loop, and in that loop we take care of refreshing the GUI. What I’m going to write now is really wrong:

this->guiManager().reportTable().update(this->reportManager().populate(
    this->dbManager().query(someSqlString)));

Well, there are many horrible thing here, but the layer violation happens in this->guiManager().reportTable().update(…). Imagine the various components of this scheme as layers on top of each other. We have the Controller, the GuiManager and a certain ReportTable.

What we’re doing, is accessing the ReportTable layer from the Controller one. Why is this bad? Having layer violations will fill your code up with disturbance. You will rapidly lose track of what does what (e.g., who is updating the ReportTable? The Controller or the GuiManager?), and this will end up into an intertwined mess commonly known as Spaghetti Code.
Doing that, you are performing actions from parts of the code to which those actions do not belong. Classes shouldn’t care about what other classes are, but only about what other classes do. Think about it: do you really want to let the Controller know that the GuiManager has a ReportTable, inside? Shouldn’t the Controller tell the GuiManager just what to do, rather than how to do it? Having classes access inner functioning of other classes will lead you to messy code, especially when there’s more than one people working on a project, as discussed later in this article. Having all the communication happening between adjacent layers will help us keeping the project consistent even in case of changes to components. Imagine if one day I will decide that the GuiManager doesn’t need a ReportTable, but a ReportChart. My ideal scenario is the one where all the changes I need to make are only within the GuiManager. But if there was a layer violation, such as the mentioned one, I would have to modify the Controller as well. When people in a group work on different components of a system, they don’t want to make a change that will break everything else. In order to avoid broken code, it would be a good practice to keep layers commnicate with the adjacent ones, according to well known interfaces.

Information hiding.

This brings us to our next point. What does the Controller need to know about the members of the GuiManager? Ideally, nothing. Ideally, there would be no getters or setters, since the Controller doesn’t need to know anything about the GuiManager‘s inner functioning. What needs to be done, in fact, is designing a well known interface for the GuiManager that the Controller can use. Once designed, such interface should never be changed, in order to ensure maximum compatibility within the components. Imagine you have just a certain GuiManager::update() method, the Controller would just need to call this->guiManager().update() and, whatever the GuiManager does, is none of the Controller‘s business. Inside, the GuiManager might do something like this->reportTable().update(), but in case this would change to a ReportChart, it wouldn’t break the Controller, and keep the people that work with it happy.

Abusing singleton pattern.

Singletons are not a way to get yourself some global variables. Think thoroughly about the reasons why you really need a Singleton in your program. Is it just a way to access some variables from everywhere in the code? If the answer is yes, you should consider refactoring your code to get rid of the Singleton class. Keep also in mind that Singletons are enemies of unit testing. Have a Singleton class do something, rather than contain something. A typical example of a class suitable to be a Singleton is a Logger class. You need to access it from everywhere in the code; the class doesn’t need to be aware of the application it’s in; the class does (logs) and doesn’t just contain. If you write a Singleton class like the following, you’re doing something wrong:

class AccessData : public Singleton<AccessData> {
    friend class Singleton<AccessData>;
    public:
        std::string username;
        std::string password;
};

This class seems to have the sole purpose of easing the access to a certain username and password from everywhere in the code, without the need of passing them around. You should consider passing references and data around only when needed, or adopting some signaling framework.

Having been coding for 16 years now (I started quite young), I have seen a lot of bad code. Code is not good just because it works. So here’s a quick list of 10 advice that you’d better keep in mind while coding.

1. Don’t sacrifice code maintainability to performance, unless it’s strictly necessary.

   This happens very often. You have to consider that your code is likely to be read by many persons, and some of them will read it after you might have parted from that company. Remember that you won’t remember what your own code does after few weeks. So always try to put things in the most readable and obvious form, even if this will require writing more lines of code, or having less performing code. Of course this is not so important if performance is your number one issue. Try, for instance, to avoid use of the ?: operator in C/C++. Everybody will understand it anyway, but a good old if statement will do it, so why not going for it?

2. Be precise as a Swiss clock, when it comes to naming conventions.

   Nobody wants to read class names or variable names that look like gibberish. Don’t be mean on the keyboard: when you type, remember that somebody else will have to read it, so be extensive.

   1. Name your variable NumberOfItems rather than items_n. Don’t use cryptic prefixes to class name. Name your class ClientMessageOperationsBasicFunctor rather than CMOpFunctor. It’s a lot more typing for you, but a lot less hassle reading for the ones that will come after you.

   2. Don’t change your conventions. If you’re calling the iterators i, don’t call any of them n, ever. You will induce confusion to your reader. It doesn’t seem as important as it actually is. If you call a class ClientMessageBlockContact, then do not have ServerMessageContactBlock. Be perfect, be precise, be obsessed.

3. Use a good and consistent indentation style.

   Never ever have more than one blank line. Don’t have trailing spaces at the end of the lines. Don’t have blank spaces or TAB characters in blank lines. A blank line must be blank, that is.Be consistent: don’t use TABs to indent in one file, and spaces in another one. Possibly, use 8-chars wide TABs to indent. If you find yourself going beyond 80 rows too often, then that could be an indication that there might be some design flaws in your program. Tweak your editor to show you the end-of-line character and the TABs.

4. One class, one file.

   Don’t write files like ServerMessages.h where you write all the class that are ServerMessages. One class goes in one file. If you find yourself thinking that you can’t do it, review your design.

5. In C/C++, project includes use “”, dependency includes use <>.

   If you’re including a file that’s local to your project, use #include "file.h"; if it’s an external dependency, do #include <file.h>. Why? I’ve seen people including <file.h> and then just put things like /usr/includes/my_project in the inclusions search path, so that nobody will be able to compile before installing. That’s a bad assumption. And you don’t want to end up in that error.

6. Always compile with -ansi -pedantic -Wall -Werror flags (or similar, according to your compiler).

   Let’s adhere to standards. Let’s avoid warnings. A warning might become an error in the future.

7. Use TODOs and FIXMEs.

   If you know that you, or somebody else, will have to return on a certain piece of code to add or modify some functionality, please mark it with a TODO. If you know that a piece of code is buggy but you can’t fix it right now, add a FIXME marker. Later on, it will be easy to grep the source tree for TODOs and FIXMEs and analyze them, especially if they’re very well commented.

8. Comment your own code.

   Seriously: you’re going to forget, sooner than you think. Just invest 5% of your time in writing commented code. Never assume that code is self-explanatory, just write a couple of lines for everything you do. Comments are not only meant to generate doxygen documentation. You have to assume that somebody else will read your code and need to modify/extend it.

9. Use a versioning system even if you’re working alone.

   Yes, versioning is not just for working in a team. Use Darcs or SVN even if you’re working alone: you won’t regret it.Commit often and try to be professional all the time. Later on somebody else might join you. Or then you might find useful to revert to previous versions of your program. And it will help you to keep trace of what you’re doing.

10. Use a bug tracking system even if you’re working alone.

    Things like Bugzilla are EXTREMELY useful. Usually you will forget bout a bug after less than 2 days. Everytime you find a bug, either fix it immediately, or mark it to your personal bugzilla. And always fix bugs first, and then write new code.

Common errors:

• It compiles, so it works.
• It works here, so it works everywhere.
• Commenting? We don’t have time to waste, we gotta ship!
• UML diagrams are useless.
• Plan code so that we can reuse it is useless: we’ll end up writing everything from scratch anyway.
• Unit tests are a waste of time.
• …