8

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?

  • OOP is about separating concerns, for example separating client from implementation via an interface -- but also separating choice of implementation from usage of an interface, while also enabling multiple implementations that potentially dynamically coexist. Using @CandiedOrange's strategy example, the chooser of the strategy and consumer of the strategy can be separated; whereas with static methods, even though the client and implementation concerns are separated, there is tight coupling of the client to the choice of (the single) implementation. – Erik Eidt Nov 11 '16 at 18:32
  • 2
    It's bad design that I can do IO("somefile.txt").get_mtime("someotherfile.txt"). What does that even mean? – immibis Sep 28 '17 at 22:23
8

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.

  • 1
    +1 In regards to statics "This thinking has the implication arrow going in the wrong direction." is perfect. I used to work at a company where if a function didn't need state it would always be static, which meant like 60% or so (if not more) of the app ended up being static. Talk about a nightmare. – Carson Mar 16 '17 at 20:18
  • Really? We do the exact opposite at my current company (and I advocated for it). Any method that can be made static is. – gardenhead Mar 17 '17 at 3:59
  • @gardenhead If you wish to make a counter argument please make it. Telling me a whole company is doing it doesn't mean it's a good thing. – candied_orange Mar 17 '17 at 4:03
  • 1
    @candiedorange I agree it's not proper evidence, but I didn't want to get into a flame-war. But since you insist, here's my counter argument: OOP is terrible and the less of it used the better. Happy? – gardenhead Mar 17 '17 at 4:04
  • @gardenhead OOP is terrible so static is good? I can write pure functional code without ever once using statics. Are you sure you aren't looking for a flame-war? – candied_orange Mar 17 '17 at 4:08
5

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.

  • As mtime doesn't require to open file descriptor this is the reason I wanted to make it independent of member variable m_file_name. Therefore, it makes more to make it static in the class I understood that part. But I want to understand from academic/theortical point of which OPPs concept I violate with the 1st option. – Rahul Nov 10 '16 at 19:05
  • 2
    @Rahul There really isn't a good answer for that. I mean, I suppose I could say that it violates the abstraction by implying a dependence on instance data, but that's not really what abstraction is about. Honestly, I think you're worrying too much about the whys. Just take it as a general rule of thumb that if a member function doesn't need to access an instance, then it shouldn't be a non-static member. – greyfade Nov 10 '16 at 23:08
  • 1
    +1 but yes practically I knew it was not a good idea but i wanted to clarify my understanding "why" ? CandidOrange's answer explain it. – Rahul Nov 11 '16 at 5:08
2

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.

2

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.