Check in constructor vs. set function

Question

this is a very simple question. Suppose I have a class

class A
{
public:
  A(int);
  void setA(int);
private:
  int a;
};

with implementation

A::A(int a_) : a(a_) { }
void A::setA(int a_) { a = a_; }

Let's say I want to avoid having a = 0 but I don't want to throw an exception, I just want to keep a valid object with a = 1 instead in that case (it doesn't matter why right now).

Then I can add an if statement in the constructor and in setA to check whether the parameter is 0, and set it to 1 in that case. Conceptually, this tells me that in that case I should change the constructor, and instead of initializing a there, I should call setA in the constructor. In this way, the code for the check is only written once (keep in mind that this a simple situation but the validation could be more complicated in other situations).

Now, that approach involves having an extra function call in the constructor. Is it more efficient to write the validation code twice? What about if setA were only to be used sporadically?

Will probably be inlined anyway, add it in `setA` and call `setA` in constructor. Stop worrying about these (what seems to be but actually aren't) performance differences. Write clean code. The optimizer will do the rest for you. — Hatted Rooster, Feb 14 '17 at 12:44
You don't need to rename parameters: `A::A(int a) : a(a) { }` is fine. — nwp, Feb 14 '17 at 12:48
@nwp Wow, I did not know that. I was going to ask a language lawyer question to find out when this took place, but apparently this has been a thing for a while: http://stackoverflow.com/q/6185020/2642059 — Jonathan Mee, Feb 14 '17 at 13:47
@GillBates I've already taken advantage of it twice. My code suddenly feels so savvy. — Jonathan Mee, Feb 16 '17 at 14:39

score 1 · Answer 1 · answered Feb 14 '17 at 13:23

1

It's definitely better to call setter with validation in constructor, because code duplication is evil. You can also make it inlined to make sure you are not wasting CPU cycles, but this is a premature optimization, I think.

answered Feb 14 '17 at 13:23

dmsovetov

299
1
8

1

Modern compilers don't listen to `inline`. – Hatted Rooster Feb 14 '17 at 13:25
There are compiler-specific attributes to force inlining AFAIK, like __forceinline for VS – dmsovetov Feb 14 '17 at 13:33
@GillBates Aside from making your code non-portable, `__forceinline` tells the compiler to ignore it's better judgement and use yours. Please refrain from using such `__forceinline`. – Jonathan Mee Feb 14 '17 at 13:50

score 1 · Accepted Answer · answered Feb 14 '17 at 13:27

I agree with your fervor to prevent code duplication. Just be careful of going overboard, a 1-liner might be overboard.

If you're going to do the check though you should use an implementation file function or a private, static method to do this:

int preventZero(const int a_) { return a_ == 0 ? 1 : a_; }

Then you can use it in your implementation as follows:

A::A(int a_) : a(preventZero(a_)) { }
void A::setA(int a_) { a = preventZero(a_); }

Check in constructor vs. set function

2 Answers2