Is it okay and/or normal to use #include to put large chunks of repetitive code into a separate file?

Question

I have been dissecting some code and I saw something I haven't seen before, and I was wondering if it is a good/bad practice and if it is normal.

Basically there is a header file with a class definition for a class with a bunch (around 90) of pure virtual functions. There are a lot of these virtual functions, so they are all put into a separate file and then included in the class definition like so:

Foo.h

class Foo
{
public:
    virtual ~Foo() {};
    #define FOO_VIRTUAL_IMPL = 0
    #include "Foo_prototypes.h"
};

Foo_prototypes.h

#if ! defined(FOO_VIRTUAL_IMPL)
# define FOO_VIRTUAL_IMPL
#endif

virtual void doSomething() FOO_VIRTUAL_IMPL;

virtual void doSomethingElse() FOO_VIRTUAL_IMPL;

Also

Is using the define macro like that also common (i.e. allowing the same include file to be used for pure virtual and normal virtual functions)? Is this sort of stuff used often, or just for little hacks to save a little bit of time/effort?

I suppose these things made the code seem less readable to me, but it may just be because I am not used to these tricks and once I get used to them I will be better equipped to read this sort of code.

The code in question is Interactive Brokers' C++ API, if anyone cares to see it in context. Relevant files are: EWrapper.h and TestCppClient.h and EWrapper_prototypes.h.

Commingle

Using FOO_VIRTUAL_IMPL makes your code idiosyncratic and harder to read. What's wrong with = 0;? – Pantie

I don't think this is common, but I imagine that I'd do something like this if list of this functions was generated by some external tool (possibly only by addition of new ones). Then it would makes sense, since you'd be able to add function once and have it added to all including classes. But I'd say it is once in a lifetime usage, if not rarer. – Dragonnade

@Pantie Yes, it is idiosyncratic, but exactly as Stephen Luke Czaus said this gives you ability to remove this "= 0" if including class wants to implement these functions. Macro gives you this ability and explicit "= 0" does not. – Dragonnade

Yes, it is normal to place type definitions in a header file, including class definitions, particularly if they are used in multiple source files (which all include the header). It is less common (although possible with inlining) to put the definition of class member functions into a header file - since having a single definition of member functions in one source file is sufficient. In C++, however, use of macros that expand to code is often (not always, but more often than not) considered bad practice, so using macros that expand to code is somewhat rare for anyone except beginners. – Resolve

Although you can do it, and I have seen professionals actually do it ... I would not call it normal nor okay. I recommend against. (Full admission: I consider the preprocessor The Devil, and have disdain for preprocessor abuses.) – Nevertheless

@JędrzejDudkiewicz - defining a pure virtual function is perfectly legal in standard C++ - it is not necessary to remove the =0 for that. A class with a pure virtual function cannot be instantiated (so it is necessary to derive from it, and override all pure virtuals - and optionally call the base class version that has been defined). – Resolve

So what I'm getting is that the #include is good, but the macro is bad. Is this more or less true? – Commingle

@Resolve I'm perfectly aware. If you include this file in multiple classes and some of them are meant to be abstract and others not (so they must implement these methods), then such macro makes far more sense than "= 0", which prevents you from using it with classes that are meant to be instantiated - you'd have to repeat this list by hand, which defeats the purpose. It seems that this is exactly what OP's code is willing to achieve. – Dragonnade

@StephenLukeCzaus I wouldn't put it that way. Neither is good nor bad. You can check yourself whether it is good or bad in a simple way - look at this code and check what would it take to achieve the same code/functionality without using these includes and macros. There is a chance, that someone did it just because it seemed like a cool trick, but this isn't necessarily true - I had few projects where I used similar (not identical, though) techniques, but this code was, and it is important, GENERATED. – Dragonnade

One could also ask why there are the same functions in different classes. Usually one would use an interface in the form of a common base class for this, which also gives you additional features compared to independent classes that just happen to have the same functions. Maybe the overall design could be improved here, but we don't know enough about the structure of the codebase to answer that. – Riddle

@JędrzejDudkiewicz - you missed my point entirely. I'll leave it there, since you are obviously attached to this sort of crappy technique, and not interested in the fact there are less error-prone alternatives. – Resolve

@Resolve I'm not attached to it. All I say is like most such solutions it has its uses, albeit very rarely. Would you point out where I missed the point? I agreed with all you wrote and thought I understand it, so I'd like to know what I missed. – Dragonnade

Answer 1

A safer implementation would be:

#define FOO_PROTOTYPES(FOO_VIRTUAL_IMPL)\
virtual void doSomething() FOO_VIRTUAL_IMPL;\
virtual void doSomethingElse() FOO_VIRTUAL_IMPL;

class Foo
{
public:
    virtual ~Foo() {};
    FOO_PROTOTYPES( = 0 );
};

class FooImpl : public Foo
{
public:
    virtual ~FooImpl() {};
    FOO_PROTOTYPES( override );
};

Everything is in one header and avoids accidental use of the FOO_VIRTUAL_IMPL value defined in one header in another one.

However if your class has enough methods to make such constructs worthwhile its probably time to refactor your class into smaller classes.

Epps

I think you missed this part: "class definition for a class with a bunch (around 90) of pure virtual functions". Around 90 functions. This is why #include is used. – Dragonnade

This makes it impossible to reuse the code in multiple spots, which is part of why it was in a separate header file. The macro was to allow it to be used in one spot as pure virtual functions and the other as normal virtual functions. Also the question was more on whether this is common practice, and if it is normal. You offered a different implementation, but didn't answer the questions. – Commingle

@JędrzejDudkiewicz what stops this from being used in multiple places? If the macro is defined in the header of the base class it can be used in any other header that is defining derived classes – Epps

@StephenLukeCzaus my macro can be used as many times as you like in different places – Epps

@AlanBirtles Clearly, thank you! You're right that is a lot more elegant. – Commingle

@AlanBirtles Yes, you can define it in any header, but the question is whether you want it to stay forever defined (i.e. till the end of compilation of current unit). To be honest I wouldn't change it to macro because at this point this is beyond salvation and changing it quite probably won't make a difference, but I'd consider it when creating such solution from the start. – Dragonnade

Answer 2

Wellll, this isn't exactly what I'd call well-styled code. Yes, technically you can include headers in that kind of place, but it's not exactly standard, so I wouldn't recommend it. Just in general, "stay away from the dusty corners of a language". However, if you really want to do that kind of thing, you can. However, not following standard practices can have two effects.

1. Non-standard code is generally harder to read, so less people will be able or willing to contribute.

2. Non-standard code can have more bugs that aren't as noticed, just because it's not standard.

An example of the bugs I mentioned beforehand is the way FOO_VIRTUAL_IMPL works. #define's aren't limited to scope, so this would be visible to all your code. It would be really really easy to #define it in one header, and not define it at all in another header. This would cause all the virtual functions in the second header to be purely virtual, probably not what you intended.

EDIT: Also, as Caleth said, if your class requires that much repetitive code, it would be good to redesign your class entirely.

Heptagon

Answer 3

There are several issues here.

If you have a class with the same pure virtual functions over and over again, it's an interface by defintion. There is absolutely no reason not to wrap things up in a well-defined, named interface. Modern IDE's will support you when implementing those interfaces (so there is no need for macro-magic).

Generally, using macros is bad. We wouldn't accept macro-programming in any code-review. Check out the C++ ISO guideline for more info:

Macros are a major source of bugs. Macros don't obey the usual scope and type rules. Macros ensure that the human reader sees something different from what the compiler sees. Macros complicate tool building.

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Res-macros

Behl

Answer 4

I'd be very wary of an #include that didn't appear at :: scope, especially given that it is a file ending .h.

I'm also wary of the #define, particularly as you don't then #undef it after the #include.

If you find yourself writing multiple classes that implement tens of virtual methods on one abstract base, I would first reconsider the class design. You can probably split your big interface out into a collection of related interfaces.

Edda

I agree I thought the lack of an undef after the #include was an error (in fact I'm pretty certain it is) – Commingle