CODEWITHSUNDEEP
c++c++11
Charles Marceau
Charles Marceau

Reputation: 93

Is using non-member function a good practice?

Let's say I have a class where I have multiple functions with similar logic. Since I don't want to repeat myself, I extract the similar logic in a function. Is it good practice to have similarLogic as a non-member function if it does not use any class members? Or is there a better way to do this?

Please note that in my case, the similarLogic function is strictly specific to MyClass so it won't be used anywhere outside of it.

Example with a non-member function:

MyClass.h

class MyClass {
public:
    int func1();
    int func2();
};

MyClass.cpp

int similarLogic(int p_num){
    return 5 + p_num;
}

int MyClass::func1() {
    return similarLogic(1);
}

int MyClass::func2() {
    return similarLogic(2);
}

Upvotes: 4

Views: 873

Answers (3)

sunside
sunside

Reputation: 8249

Usually, a method on a class is there to change the state of the instance, whereas a (free) function is hopefully free of side effects. (I'm not saying is has to be, but it should be.) In this spirit, there is absolutely nothing wrong with putting repeated code in a free function somewhere, even more so if it's stateless and free of side effects.

There is another layer your question can be answered on, however. The moment your class is compiled, its layout forms a binary interface that every code using your class is linked against. The moment you change your class by modifying member order, introducing variables or adding methods, you will introduce breaking changes to every caller. In the best case, it's only dependent code that is recompiled, but if your class is exposed through a header file and linked against through a shared or static object, you're up for trouble - regardless of whether your addition was private or not.

The Private Implementation (PIMPL) idiom

To remedy this, the "PIMPL" idiom can be used, which is described e.g. on cpppatterns.com/patterns/pimpl.html. In essence, the idea is the following:

Instead of having private method or member declarations in your class definition, you only provide public methods and a single pointer to an anonymous class. This "anonymous" class is only defined in the .cpp file backing your class declaration. It's not visible to the outside world, and everything private to your logic is implemented there.

To borrow from cpppatterns, this is how it looks like:

In the header file, you're defining your class foo only with members that are publicly accessible and a single private pointer pimpl of an anonymously declared type foo::impl.

// foo.h - header file
#include <memory>
class foo
{
  public:
    foo();
    ~foo();
    foo(foo&&);
    foo& operator=(foo&&);
  private:
    class impl;
    std::unique_ptr<impl> pimpl;
};

In the code, you're both defining the anonyous class foo::impl, and make every public method of foo call out to the private implementation in foo::impl. It's this foo::impl class that's carrying your actual state:

// foo.cpp - implementation file
class foo::impl
{
  public:
    void do_internal_work()
    {
      internal_data = 5;
    }
  private:
    int internal_data = 0;
};

foo::foo()
  : pimpl{std::make_unique<impl>()}
{
  pimpl->do_internal_work();
}

foo::~foo() = default;
foo::foo(foo&&) = default;
foo& foo::operator=(foo&&) = default;

If you try this, note that the last three lines here are key to make it work, as the constructor, destructor and copy operator can only be defined when the type foo::impl is known.

Very much like your idea with the free, "private" functions, no changes introduced to the core logic in foo::impl will ever be observable by any user of foo, and you get to have class methods operating on state.

The cost, on the other hand, is that of introducing more code to write and maintain, and that of (at least theoretically) passing the calls along the private pointer to the actual implementation.

So ... pick the right tool for the job. The one you thought of is fine. :)

Upvotes: 3

Clonk
Clonk

Reputation: 2070

If the functionality is only used by MyClass then the function should be a private member function.

If it does not use any MyClass attributes you can eventually make it static, but it's not necessary.

If it's similar logic that can be applied to different type, you can also use a template function. For instance : 

// MyClass.h

class MyClass {
public:
    int func1();
    int func2();
private:
    template<class T> void similarLogic(T arg); 
};

template<class T> MyClass::similarLogic(T arg) {
    // Generic implementation
}
template<> MyClass::similarLogic<float >(float arg) {
    // Specialization for int
}

// MyClass.cpp
int MyClass::func1() {
    // Call generic implementation for int
    return similarLogic<int>(1);
}

int MyClass::func2() {
    // Call specialization for float
    return similarLogic<float>(2.34);
}

Upvotes: 2

Bathsheba
Bathsheba

Reputation: 234695

Personally, I'd write similarLogic in an anonymous namespace within MyClass.cpp. In that way it's invisible outside that translation unit:

namespace {
    int similarLogic(int p_num){
        return 5 + p_num;
    }
}

Other approaches such as a private member of the class (static or otherwise) pollute the class definition. But that doesn't make the approach a poor choice. If the function requires class member data in the future, then refactoring is simpler than it would be with my favoured way.

Whatever you end up doing, polluting the global namespace is not desirable.

Upvotes: 9

Related Questions