Which is best for a repeating piece of code?

Question

I have a class with two member functions that share a piece of code:

void A::First()
{
   firstFunctionEpilogue();
   sharedPart();
}

void A::Second()
{
   secondFunctionEpilogue();
   sharedPart();
}

Currently firstFunctionEpilogue(), secondFunctionEpilogue() and sharedPart() are not function calls but just pieces of code, sharedPart() code being duplicated. I want to get rid of the duplication.

The shared piece of code doesn't need access to any members of the class. So I can implement it as any of the three:

• a static member function,
• a const non-static member function or
• a local function.

Which variant is better and why?

Answer 1

If your function accesses state but does not change it then use a const member function.

Your case:

If it your function 1) doesn't need access to any member of the code, and 2) is related to that class, then make it a static function of your class.

That way it is clear that it is not modifying state, nor based on the state of the object.

An extra case you didn't mention:

There is another thing you can do too. And that is to make your SharedPart take in a member function pointer and to call it and then process it's main body. If you have a lot of First(), Second(), Third(), Fourth(), ... such functions then this can lead to less code duplication. That way you don't need to keep calling SharedPart(); at the end of each member function, and you can re-use First(), Second(), THird(), ... without calling the SharedPart() of the code.

Answer 2

Or it could be in a different class.

Or, if it's a member, it could be virtual.

There are a lot of decisions, and I wouldn't stress out about it too much. Generally, I opt for a const non-static member function as a default unless I have a good reason not to do it that way.

1. Prefer static if clients need to call it without having an instance
2. Prefer local functions if you don't want to clutter the .h file or you want it completely hidden in the .c

Answer 3

I'd say:

• It probably doesn't matter, so it's not so much "best practice" as "just don't do anything crazy".
• If the class and all its members are defined in its header, then a private static member function is probably best, since it clearly indicates "not for clients". But there are ways to do this for a non-member function: don't document it, put in a comment "not for clients", and stick the whole thing in namespace beware_of_the_leopard.
• If the class member functions are defined in a .cpp file, then little helper functions like this are best as free functions in the .cpp file. Either static, or in an anonymous namespace.

Answer 4

a static member function, a const non-static member function or a local function.

Generally, it should be a member function of another class, or at least non-static member of the class itself. If this function is only called from instance members of a class - probably its logical meaning requires an instance, even if syntax does not. Can anything except this object provide meaningful parameters or make use of the result?

Unless it makes sense to call this function from outside of the object instance, it shouldn't be static. Unless it makes sense to call this function without accessing your class at all, it shouldn't be local.

Borrowing examples from Brian's comment: if this function changes global state, it should be member of a class of global state; if this function writes to file, it should be member of a class of file format; if it's refreshing screen, it should be member of... etc Even if it's a plain arithmetic expression, it may be useful to make it a member (static or not) of some ArithmeticsForSpecificPurpose class.

Answer 5

Make it a non-member function

The shared piece of code doesn't need access to any members of the class.

As a general rule, if a piece of code doesn't need access to any members of the class don't make it a member function! Try to encapsulate your classes as much as possible.

I'd suggest doing a non-member function in a separate namespace that would call the public methods and then call the function you made for the shared code.

Here is an example of what I mean :

namepsace Astuff{
  class A{...};

  void sharedPart(){...};

  void first(const A& a);
  void second(const A& a);
}

void Astuff::first(const A& a){
   a.first();
   sharedPart();
}

Answer 6

Make it a non-member non-friend function. Scott Meyer's has a great explanation for this here (and also Item 23 of Effective C++ 3rd Edition).

Answer 7

As a rule of thumb "try to keep it as local as possible but as visible as necessary".

If all code calling the function resides in the same implementation file, this means keeping it local to the implementation file.

If you'd make it a private static method of your class, it would not be callable by implementaions including your class, but it would still be visible to them. So every time you change the semantics of that method, all implementaions including your calls will have to recompile - which is quite a burden, since from their point of view, they don't even need to know those sementics.

Thus, in order to minimize unnecessary dependencies, you would want to make it a static global function.

However, if you should ever find yourself repeating this global function in mulitple implementation files, it would be time to move the function into a seperate header/implementaion file pair, so that all callers can include it.

Whether you place that function into a namespace, at global scope, or as a static function in a class is really up to taste.

On a final note, if you go for the global static function, there's a "more c++ like" version: anonymous namespaces. It has the nice property that it can actually store state and also prevents users for being able to even forward declare any of its functions.

// in your .cpp file
namespace /*anonymous*/
{
   void foo()
   {
     // your code here
   }
};

void MyClass::FooUser1() { foo(); }
void MyClass::FooUser2() { foo(); }