Salvatore Iovene

Today I want to talk about Object Ori­ented prac­tices, and 3 com­monly made mis­takes. Very often, when review­ing code writ­ten by other peo­ple, I find vio­la­tions of com­mon OO prac­tices, that make the code a lot less main­tain­able. Here fol­lows a list of the most com­mon ones, and, of course, some expla­na­tions about them.

Layer vio­la­tion.

While not the most com­mon, this appears to me as the most dan­ger­ous. What is layer vio­la­tion? Let’s show it with an exam­ple. Assume we have a GUI dri­ven appli­ca­tion that reads data from a data­base and shows the results on the dis­play. We might con­sider hav­ing some upper level Con­troller class, and man­agers for each com­po­nent, e.g. GuiManager, DbManager, ReportManager. Assume that the Controller class runs a loop, and in that loop we take care of refresh­ing 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 hor­ri­ble thing here, but the layer vio­la­tion hap­pens in this->guiManager().reportTable().update(…). Imag­ine the var­i­ous com­po­nents of this scheme as lay­ers on top of each other. We have the Controller, the GuiManager and a cer­tain ReportTable.

What we’re doing, is access­ing the ReportTable layer from the Controller one. Why is this bad? Hav­ing layer vio­la­tions will fill your code up with dis­tur­bance. You will rapidly lose track of what does what (e.g., who is updat­ing the ReportTable? The Controller or the GuiManager?), and this will end up into an inter­twined mess com­monly known as Spaghetti Code.
Doing that, you are per­form­ing 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? Hav­ing classes access inner func­tion­ing of other classes will lead you to messy code, espe­cially when there’s more than one peo­ple work­ing on a project, as dis­cussed later in this arti­cle. Hav­ing all the com­mu­ni­ca­tion hap­pen­ing between adja­cent lay­ers will help us keep­ing the project con­sis­tent even in case of changes to com­po­nents. Imag­ine if one day I will decide that the GuiManager doesn’t need a ReportTable, but a ReportChart. My ideal sce­nario is the one where all the changes I need to make are only within the GuiManager. But if there was a layer vio­la­tion, such as the men­tioned one, I would have to mod­ify the Controller as well. When peo­ple in a group work on dif­fer­ent com­po­nents of a sys­tem, they don’t want to make a change that will break every­thing else. In order to avoid bro­ken code, it would be a good prac­tice to keep lay­ers comm­ni­cate with the adja­cent ones, accord­ing to well known interfaces.

Infor­ma­tion hiding.

This brings us to our next point. What does the Controller need to know about the mem­bers of the GuiManager? Ide­ally, noth­ing. Ide­ally, there would be no get­ters or set­ters, since the Controller doesn’t need to know any­thing about the GuiManager’s inner func­tion­ing. What needs to be done, in fact, is design­ing a well known inter­face for the GuiManager that the Controller can use. Once designed, such inter­face should never be changed, in order to ensure max­i­mum com­pat­i­bil­ity within the com­po­nents. Imag­ine you have just a cer­tain GuiManager::update() method, the Controller would just need to call this->guiManager().update() and, what­ever the GuiManager does, is none of the Controller’s busi­ness. Inside, the GuiManager might do some­thing like this->reportTable().update(), but in case this would change to a ReportChart, it wouldn’t break the Controller, and keep the peo­ple that work with it happy.

Abus­ing sin­gle­ton pattern.

Singletons are not a way to get your­self some global vari­ables. Think thor­oughly about the rea­sons why you really need a Singleton in your pro­gram. Is it just a way to access some vari­ables from every­where in the code? If the answer is yes, you should con­sider refac­tor­ing your code to get rid of the Singleton class. Keep also in mind that Singletons are ene­mies of unit test­ing. Have a Singleton class do some­thing, rather than con­tain some­thing. A typ­i­cal exam­ple of a class suit­able to be a Singleton is a Logger class. You need to access it from every­where in the code; the class doesn’t need to be aware of the appli­ca­tion it’s in; the class does (logs) and doesn’t just con­tain. If you write a Singleton class like the fol­low­ing, you’re doing some­thing wrong:

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

This class seems to have the sole pur­pose of eas­ing the access to a cer­tain username and password from every­where in the code, with­out the need of pass­ing them around. You should con­sider pass­ing ref­er­ences and data around only when needed, or adopt­ing some sig­nal­ing framework.