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

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?

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.

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).

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

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).

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.

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).

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;