Does it violate any OOP principle if a member function does not use any of class properties/member variables?

Question

I have an existing class which interacts which can open, read or write to a file. I need to retrieve a file modification for that purpose I have to add a new method

Suppose this following is my class definition where I want to add a new method.

class IO_file
{
 std::string m_file_name;
 public:
 IO();
 IO(std::string file_name);

+ time_t get_mtime(file_name);
+       OR
+ time_t get_mtime();
};

I have two options -

1. Create an empty object and then pass the file_name in the method argument for which I wish to retrieve the file modification time.

2. Pass the file name at the object construction and simply call the member function which will operate on the member variable.

Both options will serve the purpose. I also believe that second approach is better than first one. But what I don't understand is How? Is first approach bad design since it doesn't make use of a member variable? Which principle of Object oriented design does it violate? If a member function does not use the member variable, should that member function should always be made static?

Answer 1

Does it violate any OOP principal if a member function does not use any of class properties/member variables?

No.

OOP doesn't care if your member function uses, or doesn't use, class properties or member variables. OOP cares about polymorphism and not hard coding implementation. Static functions have their uses but a function shouldn't be static simply because it doesn't depend on object state. If that's your thinking fine, but don't blame it on OOP because that idea didn't come from OOP.

Is [it] bad design [to not] make use of member variables?

If you don't need to remember state from call to call there is no good reason to use state.

Which principle of Object oriented design [does] it violate?

None.

If a member function does not use the member variable then that member function should always be made static?

No. This thinking has the implication arrow going in the wrong direction.

• A static function can't access instance state

• If the function has no need to access instance state then the function can be static or non-static

Making the function static here is completely up to you. But it will make it more like a global if you do. Before going static consider hosting the function in a stateless class. It's more flexible.

---

I have here an OOP example of a member function that doesn't use class properties or member variables.

The member function (and it's stateless class):

#include <iostream>

class Strategy
{
public:
     virtual int execute (int a, int b) = 0; // execute() is a so-called pure virtual 
                                             // function. As a consequence, Strategy 
                                             // is a so-called abstract class.
};

 
Three different implementations:

class ConcreteStrategyAdd:public Strategy
{
public:
    int execute(int a, int b)
    {
        std::cout << "Called ConcreteStrategyAdd's execute()\n";
        return a + b;
    }
};

class ConcreteStrategySubstract:public Strategy
{
public:
    int execute(int a, int b)
    {
        std::cout << "Called ConcreteStrategySubstract's execute()\n";
        return a - b;
    }
};

class ConcreteStrategyMultiply:public Strategy
{
public:
    int execute(int a, int b)
    {
        std::cout << "Called ConcreteStrategyMultiply's execute()\n";
        return a * b;
    }
};

 
A place to store the choice of implementation:

class Context
{
private:
    Strategy* pStrategy;

public:

    Context (Strategy& strategy)
        : pStrategy(&strategy)
    {
    }

    void SetStrategy(Strategy& strategy)
    {
        pStrategy = &strategy;
    }

    int executeStrategy(int a, int b)
    {
        return pStrategy->execute(a,b);
    }
};

 
An example of use

int main()
{
    ConcreteStrategyAdd       concreteStrategyAdd;
    ConcreteStrategySubstract concreteStrategySubstract;
    ConcreteStrategyMultiply  concreteStrategyMultiply;

    Context context(concreteStrategyAdd);
    int resultA = context.executeStrategy(3,4);

    context.SetStrategy(concreteStrategySubstract);
    int resultB = context.executeStrategy(3,4);

    context.SetStrategy(concreteStrategyMultiply);
    int resultC = context.executeStrategy(3,4);

    std::cout << "\nresultA: " << resultA 
              << "\nresultB: " << resultB 
              << "\nresultC: " << resultC 
              << "\n";
}

Outputs:

Called ConcreteStrategyAdd's execute()
Called ConcreteStrategySubstract's execute()
Called ConcreteStrategyMultiply's execute()

resultA: 7       
resultB: -1       
resultC: 12

And all without execute() caring about the state of any object. The Strategy class is actually stateless. Only state is in Context. Stateless objects are perfectly fine in OOP.

Found this code here.

Answer 2

get_mtime would make more sense here as a standalone function or as a static function, rather than as you've shown here.

The mtime of a file is read, on most systems, from a call to lstat or similar, and doesn't require an open file descriptor, so it doesn't make sense to have it as a member function of an instantiated class.

Answer 3

The reason that second option seems instinctively better (and, IMO, IS better) is because your first option gives you an object that doesn't actually represent anything.

By not specifying the filename, your IO_file class is really just an abstract object which just happens to resemble a concrete file. If you're passing in the filename when you call the method you may as well just refactor the entire method into a free-floating pure function instead; there's no actual benefit to keeping it tied to an object that you'll need to instantiate just to use it. It's just a function; the object instantiation boilerplate is just an inconvenient added step.

On the other hand, once the filename is given any methods you call on that object are being are more like queries about a specific instance of a thing. That's better OO because your objects have actual meaning and, therefore, utility.

Answer 4

Let's translate this to C. First we our class - now it's a struct:

struct IO_file {
    char* m_file_name;
};

Let's, for the simplicity of the snippets, define a constructor function(ignoring memory leaks for now):

struct IO_file* IO_file(char* file_name) {
    struct IO_file* obj = malloc(sizeof(struct IO_file));
    obj.m_file_name = file_name;
    return obj;
}

Option #2 looks like this:

time_t get_mtime(struct IO_file*);

and used like this:

time_t mtime = get_mtime(IO_file("some-file"));

How about option #1? Well, it looks like this:

time_t get_mtime(struct IO_file* this, char* file_name);

And how is it used? Essentially, you are asking to pass junk as the first parameter:

time_t mtime = get_mtime((struct IO_file*)1245151325, "some-file");

Doesn't make much sense, does it?

C++'s object system hides it, but the object is also an argument of the method - an implicit pointer argument named this. This is what option #1 is bad - it has an argument that is unused by definition(arguments that happen to be unused, but may be used in overrides are OK). Such arguments contribute nothing while complicating the function's signature, definition and usage and confusing readers of the code which are left pondering what the argument is supposed to do, why is it OK to pass junk to it, and whether or not the fact that you are passing junk/NULL/empty object to it is the cause of the bug they are currently trying to solve. Spoiler - it isn't, but they are still going to waste time exploring that possibility.

The fact that the junk argument is implicit may lessen the effect, but it's still there, and and be easily avoided by making the method static.