A list of bad practices commonly seen in industrial projects

Author: Chloé Lourseyre

If you ever worked in a company-size software project (with numerous developers), there is a good chance that the codebase was, at least, pretty messy.

In these days, most industrial C++ developers are not experts. You will often work with developers with a Java or Python background, people who just learnt C++ at school and don’t really care that much about the language and old developers who code in “C with classes” instead of C++.

Having worked on a few industrial projects myself, I realized there are some recurring patterns and bad practices. If you happen to teach your coworkers to avoid these bad practices, you will all take a huge step toward a beautiful codebase and it will be beneficial to you, your coworkers and your project.

Here is a non-exhaustive list of these common bad practices.

Bad practice : Overly long functions

I am not one to set hard restrictions over the number of lines in a function. However, when a function reach more that a thousand lines (or even tens of thousands of lines), it is time to put a stop at it.

A function is a architectural block. If it is too big, it will be harder to understand. If is it split into different blocks, with explicit names and comprehensible comments, your mind will be able to turn its attention to each blocks in turns, which are individually easier to understand, and will put them back together to understand the globality of the function.

Sometimes, your function is just calling auxiliary functions in succession, and that’s ok. It’s short, easy to understand, and each auxiliary function, which are small, are also easy to understand.

To solve a big problem, split it in several smaller problems.

The limit I generally use is 200 lines per functions. Sometimes more, sometimes less.

Bad practice : Create classes when you don’t need to

This is something that is surprisingly fairly common, and probably due to other object-oriented languages that force you to use classes for everything.

There are two ways this bad practice can occur :

Full-static classes (sometimes with constructors)

It is easier to illustrate with an example, so here we go :

class MyMath
{
public:
    MyMath();
    ~MyMath();
    static int square(int i);
};

MyMath::MyMath()
{
}

MyMath::~MyMath()
{
}

int MyMath::square(int i)
{
    return i*i;
}

int main()
{
    MyMath mm;
    int j = mm.square(3);
    return j;
}

Here are the problematic points :

  • Why would you implement useless constructor and destructor, where you just could have used the default ones ?
  • Why would you implement a constructor and a destructor for a full-static class ?
  • Why would you instantiate an object just to call the static method of the class ?
  • Why would you use class at all, where a namespace would suffice ?

Here is what should have been written :

namespace MyMath
{
    int square(int i);
};

int MyMath::square(int i)
{
    return i*i;
}

int main()
{
    int j = MyMath::square(3);
    return j;
}

Shorter, better, smarter.

True, sometimes a full-static class can be useful, but in situations like that example, they are not.

There is no benefit in using a class where you could not. If you are worried that the namespace could be used as a class in the future (with attributes and methods), just remember this little rule that every one should know :

Do not code thinking of an hypothetical future that may or may not occur. The time you spend coding in anticipation is most certainly wasted, as you can always refactor later.

Fully transparent classes

I put this one in second because it is the most controversial.

Just to clear : the only difference between class and struct is that, by default, the members of a class are private and the members of a struct are public. This is truly the only difference.

So, if your class :

  • … only has public methods
  • … has both accessors (getter and setter) to all its attributes.
  • … has only very simple accessors.

… then it’s not a class, it’s a struct.

Here, to illustrate :

class MyClass
{
    int m_foo;
    int m_bar;

public:
    int addAll();
    int getFoo() const;
    void setFoo(int foo);
    int getBar() const;
    void setBar(int bar);
};

int MyClass::addAll()
{ 
    return m_foo + m_bar;
}
int MyClass::getFoo() const
{
    return m_foo;
}
void MyClass::setFoo(int foo)
{
    m_foo = foo;
}
int MyClass::getBar() const
{
    return m_bar;
}
void MyClass::setBar(int bar)
{
    m_bar = bar;
}

Is better written that way :

struct MyClass
{
    int foo;
    int bar;

    int addAll();
};

int MyClass::addAll()
{ 
    return foo + bar;
}

This is pretty much the same. You just withdraw a (useless) level of encapsulation for a more concise and more readable code.

The controversial part occur one the “useless” statement just above, because in a full-object mindset, no encapsulation is useless. In my opinion, this kind of structure don’t need encapsulation because it is just a data structure, and I don’t like when people overdo the concept of encapsulation.

Watch out, though, because this practice is only valid if all your attribute are in direct-access in both writing and reading. If one of our attribute need a specific accessor or you have read-only or write-only attributes, don’t use a struct (well, you can, but you need to seriously think about it).

Bad practice : Implementing undefined behavior

To say it short, an undefined behavior is a promise you make to the compiler that some behavior will never be implemented by your hand. Using that, the compiler will be able to make assumptions and optimize your code with those assumptions.

Go watch the talk of Piotr Padlewski : CppCon 2017: Piotr Padlewski “Undefined Behaviour is awesome!” – YouTube. It will teach you everything you need to know about UB.

Here is a non-exhaustive list of undefined behaviors. You need to know that list by heart in order to avoid unexpected undefined behavior in you codebase :

  • Calling main
  • Integer overflow
  • Buffer overflow
  • Using uninitialized values
  • Dereferencing nullptr
  • Forgetting the return statement
  • Naming variable starting with double underscore
  • Defining function in namespace std
  • Specializing non-user defined type in namespace std
  • Taking the address of a std function

So, it has to be said once and for all : do not ever rely on integer overflow to end a loop or for anything else. Because it does not mean what you think it means and one fateful day it will backfire hard.

Bad practice : Comparing signed and unsigned integer

When you compare signed and unsigned, an arithmetical conversion will occur that has a very good chance to distort the values, thus nullifying your comparison.

Use size_t when it’s relevant, and static_cast your variable if needed.

Bad practice : Trying to optimize the code as you write it

Yeah, that pill may be pretty hard to swallow. But here are two facts :

  • 80% of the time, the code you write doesn’t need to be optimized. Since most of your execution only occurs in 20% of your program (Pareto principle at work), the remaining 80% does not need to be optimized.
  • Optimization should not be a prior concern. You are to write your code, see the big picture, and optimize in consequence.

What is the most important is not how optimized is your program. The thing you should concern yourself about is whether your code is tidy, concise and maintainable. If it is, you can only come back later to optimize it.

Bad practice : Being too dumb

On the opposite, you shouldn’t under-optimize either.

You must know the specificities of the algorithms and the data structures in order to use the correct ones in your code. You need to understand some design patterns and be able to implement them so you don’t reinvent the wheel each time, you mustn’t be afraid to check the documentation before using a feature.

There is a good balance between coding without thinking and over-optimizing the code while you are writing it.

Bad practice : “If we need to do that later…”

I said it a few paragraphs above, but don’t plan on a hypothetical future. The only future you can be sure of is your most basic design. Your needs may change, your specs may change, what the client wants may change, anything beside the core design of your project may change. Sometimes it won’t, but often it will. Be sure to remember that.

When in doubt, be sure to ask yourself :

If I implement that later, will it cost more ?

Often, the answer is “no” or “not that much“. When so, leave the future to the future.

In the end…

Here you have a good start to write beautiful and maintainable code. If every dev on your project apply them, you will all benefit from it.

However, there are many many other basic bad practices I didn’t cover in this article, so the subject is not closed. Maybe I will publish a part 2 someday, maybe not.

Thanks for reading and see you next week!

Author: Chloé Lourseyre

8 thoughts on “A list of bad practices commonly seen in industrial projects”

  1. > `Taking the address of a std function`

    Can you please explain why it’s an UB?
    Piotr gave an example with std member functions like the vector’s `push_back`, where a new overload can be added in the future C++ versions. But in that case, the code will just stop to compile. And if it still compiles — it’s not an UB.
    Or am I missing something?

    1. Here is the full explanation: http://eel.is/c++draft/namespace.std#6

      Citation:
      > Let F denote a standard library function, a standard library static member function, or an instantiation of a standard library function template. Unless F is designated an addressable function, the behavior of a C++ program is unspecified (possibly ill-formed) if it explicitly or implicitly attempts to form a pointer to F.
      > […]
      > Moreover, the behavior of a C++ program is unspecified (possibly ill-formed) if it attempts to form a reference to F or if it attempts to form a pointer-to-member designating either a standard library non-static member function or an instantiation of a standard library member function template.

      In short, it is an UB because the documentation says so. There is not always a mechanical reason for an UB to be specified. The std has it’s own reasons. You can dive deeper into the library if you want to find out the intricacies behind that, but I don’t think it’s worth it.

      However, you talking about method overload seems a bit off-topic. Method overload and function pointers are two separate things, which have nothing in common.

      As for your last statement, “And if it still compiles — it’s not an UB”, well, it disturbs me. I don’t think what you are trying to state, because UB can only happen during runtime and always compile (by definition).

      1. For my last statement — this was only about the example with the `push_back` overloads *and* taking an address of one of them before the others were introduced (so, not completely off-topic).
        Piotr talked a bit about it in his CppCon talk.
        Here’s a short example from me: https://godbolt.org/z/socjxGc99 — it compiles in C++98, but fails to compile in C++11.

        My question is: if such code compiles, how can an UB manifest itself in practice?
        I got your “because the documentation says so” point, but maybe there’s more to it…

        I understand those things for most other kinds of UB. For instance, the compiler can just remove the check `if (this != nullptr)` from the code because `this` is never allowed to be null.

        But this particular case with taking functions’ addresses really makes me wonder. I cannot think of anything that could go wrong here. So, is this case really worth worrying about?

        1. How can an UB manifest itself in practice? Well, there is no way to know, since it’s undefined.
          You might be able to isolate a specific behavior in a specific environment, but the behavior may vary depending on the compiler, the OS, the version of the library, and have inconsistent behavior (behavior may vary from one execution to another).
          That is actually why it called “undefined behavior”.

          I would give you a list of the possibilities if I could (and if it is possible, which is unsure), but it’s a bit out of my range.

      2. Unspecified behavior is not there same thing as undefined. Herb Sutter recently wrote a post on exactly this subject.

        Also you say “integer overflow is a ub”. The behavior is defined for unsigned integers. Only signed integer overflow is a ub.

        But otherwise good article đź‘Ť

  2. Just to clarify. You wrote that there is only one difference between class and struct. But there exist the second one: type of default inheritance. Classes has private when structs has public.

Leave a Reply