2

When looking at c++ source code, I almost always see #define macros at the head of the file, this makes sense in most cases, and I can see why this is best practice, but I recently came upon a situation where it might be better to keep the preprocessor definitions in a function body.

I'm writing a quaternion class, and my code for the function in question looks like this:

Quaternion Quaternion::operator*(const Quaternion& q){
    Quaternion resultQ;

    // These are just macros to make the code easier to read,
    // 1 denotes the quaternion on the LHS of *,
    // 2 denotes the quaternion of the RHS 0f *, e.a. Q1 * Q2.
    // the letter denotes the value of the real number constant part 
    // for each seperate part of the quaterion, e.a. (a+bi+cj+dk)

    #define a1 this->get_real()
    #define a2 q.get_real()
    #define b1 this->get_i()
    #define b2 q.get_i()
    #define c1 this->get_j()
    #define c2 q.get_j()
    #define d1 this->get_k()
    #define d2 q.get_k()

    // This arithemtic is based off the Hamilton product
    // (http://en.wikipedia.org/wiki/Quaternion#Hamilton_product)
    resultQ.set_real(a1*a2 - b1*b2 - c1*c2 - d1*d2);
    resultQ.set_i(a1*b2 + b1*a2 + c1*d2 - d1*c2);
    resultQ.set_j(a1*c2 - b1*d2 + c1*a2 + d1*b2);
    resultQ.set_k(a1*d2 + b1*c2 - c1*b2 + d1*a2);
    return resultQ;
}

I decided to add in the #define because if I substituted in all the macros manually, each line would be too long, and either be cut off or carried over to the next line when read. I could have done the same thing with variables, but I decided that would be an unnecessary overhead, so I used #define because it has no runtime overhead. Is this an acceptable practice? Is there a better way to make what I am doing here readable?

user2864740
  • 60,010
  • 15
  • 145
  • 220
Nathan BeDell
  • 2,263
  • 1
  • 14
  • 25
  • It is cleaner to use inline functions. In your case, these #defines doesn't have side effects, but in other cases they may. inline functions avoid this problems, and should not be slower than using #defines. – Xavier Rubio Jansana Mar 29 '14 at 23:02
  • Note that macros are text substitution before compiling, and have no effect on runtime performance. The also are not bound by the language scoping rules. – Fred Larson Mar 29 '14 at 23:02
  • Your code is unnecessarily complex. Check my answer below for a redesign suggestion with no macros or helpers at all. – iavr Mar 29 '14 at 23:36
  • 1
    When you complete your class, consider submitting it to [Code Review](http://codereview.stackexchange.com/). You'll get more helpful feedback. – iavr Mar 29 '14 at 23:47
  • @ivar I actually coded this class to review how to write overloaded functions for the C++ course I'm taking, so I don't think a rigorous code review is in order (If I actually needed to use quaternions for anything I'd just use a library), writing this class just brought up this one specific question, but I'll keep that in mind for later, thanks! – Nathan BeDell Mar 29 '14 at 23:56

3 Answers3

3

Instead of

#define a1 this->get_real()

write

auto const a1 = get_real();

And just use different names for each value of a quantity that changes.

Yes there are cases where a local #define makes sense. No this is not such a case. In particular, since you've forgotten to #undef the macros they will almost certainly cause inadvertent text substitution in some other code, if this is in a header (as indicated).

By, the way, instead of

Quaternion Quaternion::operator*(const Quaternion& q){

I would write

Quaternion Quaternion::operator*(const Quaternion& q) const {

so that also const quaternions can be multiplied.

Cheers and hth. - Alf
  • 142,714
  • 15
  • 209
  • 331
  • 2
    That works only if the value returned by `get_real()` doesn't change during the calculations. (Maybe it doesn't, I have no idea) – Neil Kirk Mar 29 '14 at 23:05
  • None of the functions called should change value during the calculations. Also, wouldn't `auto const a1 = get_real();` not be as efficient as using `#define`? (after adding `#undef`s to the end of the function body to make sure inadvertent text substitution doesn't occur) – Nathan BeDell Mar 29 '14 at 23:12
  • 1
    @Sintrastes Why would it be less efficient? When using `#define` you're calling the function multiple times.. – user1520427 Mar 29 '14 at 23:13
  • There would be the same amount of function calls, but wouldn't using variables take up more memory? (although, I doubt that it would *really* be that much of a concern considering the memory would be freed from the stack once the function is exited) – Nathan BeDell Mar 29 '14 at 23:17
  • 2
    @Sintrastes The return value of `this->get_real()` still needs to be stored on the stack (barring register optimizations - but that's the compiler's job), the only difference is that it becomes a temporary instead of a local variable. Edit: Also, without optimization you'd be calling `get_real` each time you use `a1` – user1520427 Mar 29 '14 at 23:19
  • @user1520427 Ah, ok. With that being said, using `auto const a1 = get_real();` would be a lot most concise since you don't have to write `#define`s and `#undef`s. Makes sense now. :) – Nathan BeDell Mar 29 '14 at 23:22
1

Macros don't respect scope. Those macros are defined for the rest of the file after they appear. If you have a variable a1 in the next function, it will mess up. You should #undef all the macros at the end of the function.

Better is to create some utility functions somehow. In C++11

auto a1 = [this](){ return this->get_real(); }
...

resultQ.set_real(a1()*a2() ...

Not quite the same as you need the () but maybe good enough for you.

If the values of a1 etc don't change during the calculations, you should use Alf's suggestion.

Neil Kirk
  • 21,327
  • 9
  • 53
  • 91
1

The other answers already give alternatives to macros. You should definitely follow one such alternative because this use of macros in your code is unnecessary and bad.

But I feel your code needs redesign anyway, after which you'll see that neither macros nor their alternatives are needed.

A quaternion is a generalization of a complex number, and as such, essentially it has a number of data members (four rather than two), a number of constructors, and a number of operators.

You might have a look at the design of std::complex to get ideas. The design of a quaternion need not be much different. In particular, why would you need setters/getters to access data from a member function? These methods are exactly what makes expressions long! (along with the unnecessary use of this).

So, if the data members of a quaternion are a, b, c, d, and if there is a constructor with these four values are arguments, then your operator* should really look like this:

Quaternion Quaternion::operator*(const Quaternion& q) const
{
    return Quaternion{
       a*q.a - b*q.b - c*q.c - d*q.d,
       a*q.b + b*q.a + c*q.d - d*q.c,
       a*q.c - b*q.d + c*q.a + d*q.b,
       a*q.d + b*q.c - c*q.b + d*q.a
    };
}

No need for macros, helper functions, or intermediate variables.

iavr
  • 7,547
  • 1
  • 18
  • 53
  • That looks pretty complex, but maybe there is some rule that makes it easy to remember? Also, is it possible to express this simpler in terms of complex number operations? – Cheers and hth. - Alf Mar 29 '14 at 23:25
  • It's just a simplification of the original code. I manually substituted the macros back. Not that it's easy to remember, but at least this is closer to the mathematical definition and you can start seeing some pattern. The original code was more like obfuscated. – iavr Mar 29 '14 at 23:29
  • I was taught that it is best practice to always use set/get functions in class member functions, because if you ever need to change anything (for example, input checking in your set functions), you only have to change the code in one place. However, now that I think about it, I don't see any reason why you would need to do anything like that for a quaternion, so I guess that the use of set/get functions is pretty pointless in this case. – Nathan BeDell Mar 29 '14 at 23:36
  • @Sintrastes This is a huge discussion, but even if you have setters/getters, recall that this `operator*` is a member function that can access `a,b,c,d` directly, and also the members of `q` because this is an instance of the same class. So the above code is valid anyway. – iavr Mar 29 '14 at 23:41
  • 1
    @Sintrastes Plus, remember that we're initializing a new object here. Your code default-initializes and *then* assigns new values, which is not efficient. – iavr Mar 29 '14 at 23:44
  • @iavr Right, but my original thought here was not that I wouldn't be able to access `a,b,c,d` directly, but the fact that even though I could access them directly, it would be better to use set/gets because of modularity concerns. Once again though, I don't think there is any real reason that you would want to use a set/get within another member function in this case, so this may be an exception to the general rule of using set/gets in member functions. (see http://stackoverflow.com/questions/16968076/c-why-should-i-use-get-and-set-functions-when-working-with-classes) – Nathan BeDell Mar 29 '14 at 23:51