What can be a reason for making some class-specific functionality a separate function instead of member function?

Question

While reviewing rather old code I found some strange design decision:

// irrelevant stuff removed
class Class {
public:
   int GetStage() const { return stage; }
   void SetStage( int value ) { stage = value; }
private:
   int stage;
};

void ProgressStage( Class* object )
{
    int stage = object->GetStage();
    assert( stage < MaxStage );
    stage++;
    object->SetStage( stage );
}

now ProgressStage() is in the same header because it has to be visible at multiple call sites. It calls non-static member functions of the class. IMO it should be a member function instead:

void Class::ProgressStage()
{
    //directly access member variables
    assert( stage < MaxStage );
    ++stage;
}

What could be the reason for preferring the former design to the latter?

This might be a dumb question but: why not just put `stage` public and do: `Class c; c.stage++;` ? Well, unless `ProgressStage()` does other things you didn't talk about. — ereOn, Aug 31 '10 at 07:03
Related question: http://stackoverflow.com/questions/503664/member-functions-for-derived-information-in-a-class — Naveen, Aug 31 '10 at 07:06
@ereOn: The number one reason is the `assert()` to prevent misuse. — sharptooth, Aug 31 '10 at 07:17
Actually, the assumptions for this question are wrong: It's stupid to have a class that's only getters and setters. That's not OO, that's pseudo-OO. It's a glorified `struct` with all-public data, in a way that a Singleton is a glorified global variable - only much worse. See [this wonderful article (PDF link)](http://www.idinews.com/quasiClass.pdf) which sums it up much more eloquently than I ever could. — sbi, Aug 31 '10 at 08:55

Potatoswatter · Answer 1 · 2010-08-31T07:42:26.040

In C++, free functions are the preferred way of doing things. A class should present a minimal interface to do its job. Language features such as argument-dependent lookup work to associate free functions with operand classes. Such functions add "sugar" to the interface.

Here is a paper from Herb Sutter, a prominent member of the standards committee:

http://www.gotw.ca/publications/mill02.htm

Edit: Now that I actually stopped and read your code, that does look excessive. The only "job" that class appears to do is ensure that stage < MaxStage, i.e. to maintain an invariant on its state.

Supposing that stages can't be skipped or reversed, perhaps SetStage should be eliminated and AdvanceStage should be moved inside the class. In any case, it would appear that SetStage should contain that assertion, not the free function.

score 2 · Answer 2 · answered Aug 31 '10 at 07:40

The reason is quite simply that it is a better solution.

Why would you rather have the function be a member method? So that it can access all the private members it doesn't need?

Ask your self why the class has getters and setters, if you prefer not using them? Your suggestion is basically to bypass them for absolutely no reason.

Making it a free function much better preserves encapsulation, by minimizing the amount of code that can access the internals of the class.

It is the idiomatic solution in C++, as described here or here.

The basic rule of thumb should always be: place as much class functionality as possible in nonmember functions. The less code is able to see the internals of the class, the less code risks breaking when you change the implementation of that class.

There is nothing particularly "object-oriented" about the . operator. Using it doesn't necessarily make your code "more OOP" (or "better", for that matter).

Agree, this is what Scott Meyer explains in its Effective C++ Third Edition, Item 23: "Prefer non-member non-friend functions to member functions. Doing so increases encapsulation, packaging flexibility, and functional extensibility." — Julien-L, Aug 31 '10 at 07:46
Don't agree at all. If the developer has access to the source code of the class and can modify it without breaking other class clients, then there's no real reason to use an separate function to increment an internal variable of the class itself. Think about if the Class developer wants to change the implementation of the ProgressStage method in other versions of his Class. — vulkanino, Aug 31 '10 at 08:53

score 1 · Answer 3 · answered Aug 31 '10 at 07:10

1

A non-member function is easier (less typing) when used as a callback.

answered Aug 31 '10 at 07:10

Agnel Kurian

57,975
43
146
217

This is likely the correct answer. `ProcessStage` is a simple function, so can be passed around in places that expect a function pointer without any problems. – munificent Aug 31 '10 at 22:40

score 1 · Answer 4 · edited May 23 '17 at 10:33

1

I'd just use:

class Class { 

public:
    bounded<unsigned, 0, MaxStage> stage;
};

using the bounded template I posted in a previous answer. This does assume/require that MaxStage is a constant. If it's not, you can use a variant of the same idea that passes the bounds to the ctor instead (also useful for bounded floating point types).

edited May 23 '17 at 10:33

Community

1
1

answered Aug 31 '10 at 07:32

Jerry Coffin

476,176
80
629
1,111

score 0 · Answer 5 · answered Aug 31 '10 at 07:05

This class seems to me to be a data object (DAO) since no business logic is included. Probably they wanted to seperate the business logic from the entity tier. But I then I don't know why they didn't put the business logic into a manager class or something similiar.

score 0 · Answer 6 · answered Aug 31 '10 at 07:05

While I agree with you, there are some who think that any function that can be implemented using existing functionality from the public interface of a class should be made a non-member. I guess this is the "minimal but complete" mentality.

Personally, I agree with you--this feels a lot more natural and consistent as a member.

If Class had been a struct instead, I'd say that perhaps it was going to be exported in a C-callable interface, but that's not possible (AFAIK) with a class (unless you use a void pointer).

score -1 · Answer 7 · answered Aug 31 '10 at 07:02

-1

No good reason to prefer the function. C++ is not pure Object Oriented since it allows you to write functions instead of methods.

Also, what is the reason you are using a temporary variable for the stage? Is it for the Assert? I'd change it to:

assert(stage+1 <= MaxStage);
++stage;

answered Aug 31 '10 at 07:02

vulkanino

9,074
7
44
71

1

You know, most definitions of "object oriented" don't actually include "all functions must be members of a class" as a requirement. That's just something Java came up with. And there are several good reasons for preferring the free function. – jalf Aug 31 '10 at 07:41
@jalf I don't agree. Object orientation is an old methodology, not invented by Java nor C++ engineers. What about encapsulation? If you're writing C++ code instead of C, maybe you're interested in using OO paradigms, and the free function breaks them (as the any static method does). – vulkanino Aug 31 '10 at 08:43

What can be a reason for making some class-specific functionality a separate function instead of member function?

7 Answers7