If no extra #include required, is "this method doesn't suppose to be changed" a good reason to define a method in .h?

Question

Suppose I am developing mobile applications using C++, and I found some methods are not suppose to change once written, for example:

class MyClass{
public:
    float kmHrToms(float kmHr){
        return kmHr/3.6;
    }

    std::string toString(){
        return "this class is used to...";
    }

    .
    .
    .
}

and these methods wouldn't require additional #include when they are moved from .cpp to .h, is it a good idea to move those methods from .cpp to .h? I think the advantages are:

1. Clearly tells other team members that those methods shouldn't change

2. Team members would usually change .cpp but not .h, and hence less likely to change methods in .h accidentally, which fits open-principle more prefectly

3. Even the methods in .h are changed accidentally, the sudden change of compile time can warn programmers that they may change something that should not change

4. Usually programmers spend more time on .cpp than .h, moving some codes to .h can reduce number of codes in .cpp, which makes the .cpp more maintainable

is that true?

Answer 1

It is usually considered bad practice to include executable code in .h/.hpp files, some of the reasons are:

• If using a make type build you can end up with different versions of the code in different object files/libraries.
• Every object file will have the executable code, this leads to bloat.
• Everywhere that the include takes place will have a copy of the executable code even if it isn't executed and unreachable code will show poor coverage results.
• People don't expect the code to be there which can reduce maintainability.
• If you use a standards based static analysis tool it is likely to fail your code.

The only time that you would normally put code into include files would be for small, speed critical, "inline" code.

If code is not supposed to be touched use:

• Coding standards
• Naming conventions for files/code sections
• Clear comments
• Version control locks on the files
• Pre-commit filters
• All of the above

Answer 2

The code in a header file describes an interface – the types and functions that your compilation unit exposes. Everything that is necessary to describe this interface should be in the header. Implementation details do not belong in the header, because implementation details are not part of your interface.

So if the implementation of these functions is part of their public contract (necessarily the case for templates), then sure, leave them in the header. If a function is so trivial that there is only a single possible implementation and you want it to be inlined, go ahead. An example would be a getter method of an object that just returns a private field.

But in most cases, even simple functions should be kept in the .cpp file. This really is a better structure for your code, and as an added bonus improves compilation times.

In fact, headers often include far too many implementation details. There are strategies like the pimpl idiom to avoid making too many details (like private members) externally visible. This is extremely important in a library that has to maintain a specific binary interface, somewhat less so in an application. But in any case, keeping your header files minimal is more convenient and beneficial.

If you want to ensure that a function once written does not change its behaviour, the correct solution is not to prevent it from being edited. Instead, write unit tests to ensure the properties of the behaviour you care about. When someone removes the functions or changes its behaviour in an incompatible way, the test will break and alert you to this problem. No one can remember what every part of your code base is supposed to do exactly, so writing down and automating this knowledge with a suite of unit tests is a great way to prevent expensive mistakes later.

I'd also like to point out that you are trying to solve a people problem (other programmers might accidentally break the code by modifying this function) by hiding the problem (let's put the code where no one sees it). That does not solve your problem, it only shuffles it around. A better solution is to talk with your team so that you can reach a consensus (we agree that some functions should not be modified) and agree on a common solution as part of your coding conventions (e.g. “stable methods that should not be changed start with a comment // stable - do not change”).