Various methods manipulating same member variable versus each takes inputs and provides output [closed]

Question

Closed. This question needs to be more focused. It is not currently accepting answers.

---

Want to improve this question? Update the question so it focuses on one problem only by editing this post.

Closed 5 years ago.

Improve this question

As software design paradigm which is better? to let various methods manipulate a member variable, or define each method or function to take some inputs and provide some outputs?

For example

   class Test {

       void FooMethod1()
       {
           Foo = ...
       }
       void FooMethod2()
       {
           Foo = ...
       }
       void Method3()
       {
           FooMethod1();
           FooMethod2()
       }
    }

versus.

  class Test {

       var FooMethod1(var input)
       {
           input = ...
           return ...
       }
       var FooMethod2(var input)
       {
           input = ...
           return ...
       }
       void Method3()
       {
           var Foo;
           FooMethod1(Foo);
           FooMethod2(Foo)
       }
    }

Answer 1

You're confusing global variables for member variables.

Modifying member variables in member functions is fine, when your function is meant to modify the class state. That's kind of why OOP came about in the first place: to logically group state and behavior. If it's getting hard to keep track of whats happening to your member variables, that's not a problem in using them, the problem is your class is too large.

If you are going to be passing variables around non-stop, you might as well not use any OOP features.

Another thing to consider, is that if modifying member variables was considered unwise, then what would be the point of a setter function?

Let's look at an example of a circular buffer, written in C, swiped from Wiki page:

/* Opaque buffer element type.  This would be defined by the application. */
typedef struct { int value; } ElemType;

/* Circular buffer object */
typedef struct {
    int         size;   /* maximum number of elements           */
    int         start;  /* index of oldest element              */
    int         end;    /* index at which to write new element  */
    ElemType   *elems;  /* vector of elements                   */
} CircularBuffer;

void cbInit(CircularBuffer *cb, int size) {
    cb->size  = size + 1; /* include empty elem */
    cb->start = 0;
    cb->end   = 0;
    cb->elems = calloc(cb->size, sizeof(ElemType));
}

void cbFree(CircularBuffer *cb) {
    free(cb->elems); /* OK if null */
}

int cbIsFull(CircularBuffer *cb) {
    return (cb->end + 1) % cb->size == cb->start;
}

int cbIsEmpty(CircularBuffer *cb) {
    return cb->end == cb->start;
}

/* Write an element, overwriting oldest element if buffer is full. App can
   choose to avoid the overwrite by checking cbIsFull(). */
void cbWrite(CircularBuffer *cb, ElemType *elem) {
    cb->elems[cb->end] = *elem;
    cb->end = (cb->end + 1) % cb->size;
    if (cb->end == cb->start)
        cb->start = (cb->start + 1) % cb->size; /* full, overwrite */
}

/* Read oldest element. App must ensure !cbIsEmpty() first. */
void cbRead(CircularBuffer *cb, ElemType *elem) {
    *elem = cb->elems[cb->start];
    cb->start = (cb->start + 1) % cb->size;
}

The C example does pass everything in via arguments, transported around via a struct. This is done because C doesn't offer any built-in OOP features, so that's what you have to do.

If we make that in C++, making everything in the struct a member variable and accessing it 'globally' is entirely appropriate:

class CircularBuffer
{
  private:
    int         size;   // maximum number of elements           
    int         start;  // index of oldest element              
    int         end;    // index at which to write new element  
    ElemType   *elems;  // vector of elements                   
  public:
    CircularBuffer(int cbSize) {
      size = cbSize + 1; // include empty elem
      start = 0;
      end = 0;
      elems = new ElemType[size];
    }

    ~CircularBuffer() {
      delete[] elems;
    }

    bool IsFull() {
      return (end+1) % size == start;      
    }

    bool IsEmpty() {
      return end == start;
    }

    void Write(ElemType *elem) {
      elems[end] = *elem;
      end = (end + 1) % size;
      if (end == start)
        start = (start + 1) % size; //full, overwrite
    }

    //Read oldest element. App must ensure !IsEmpty() first.
    void Read(ElemType *elem) {
        *elem = elems[start];
        start = (start + 1) % size;
    }
}

If you instead had methods that did something like this:

  void Write(ElemType *elem)
  {
    _Write(ElemType *elem, this->start, this->end, this->size);
  }

or worse, had to call the member like so:

  cb.Write(&elem,cb.start,cb.end,cb.size);

there would be much rage.

The point is that the variables start, end, size, and elems are part of the internal state of the class, and so those can be manipulated directly inside the classes methods. The user of the class can only manipulate them through the exposed functions, as part of Information Hiding. If you are just going to pass the variables around to all the member functions, like you're writing C, then you might as well use C properly instead of compiling it with a C++ compiler.

Answer 2

The first is marginally better, but in general, you should try to avoid unnecessary side effects altogether.

Why? Modifying input parameters is vile. It is unexpected, it is difficult to test, it is difficult to reuse, it creates problems in concurrent environments...

Modifying a class is sometimes fine, that is what classes are for really... but it can be difficult to test, difficult to modify/extend, it creates problems in concurrent environments... And you usage here where Foo is just some placeholder to pass around in Method3 defeats the point of the class.

A better approach would be something like this:

Foo Method1(Foo x){ ... }
Foo Method2(Foo x){ ... }
Foo Method3(){
    Foo stuff = ...;
    stuff = Method1(stuff);
    stuff = Method2(stuff);
    // and return or just do stuff with stuff...
}

Answer 3

First case is hugely better. BUT only as long as the containing class is cohesive. Using first option when the class has many different responsibilities will introduce unwanted bindings which make understanding the flow of the behavior of the class worse than in second case.

Also, the first approach will have an advantage, when you have many different variables to work with and when different functions might take different variables. This would mean all functions would need to get all variables, even though only few are used by function itself and rest is just passed into other methods.

There is one example, where the case of member variables is end result of refactoring. It is when you have complex and long method, but refactoring it into individual method calls would introduce tons of method parameters being passed around. In this case, you turn the whole method into a single class and turn the method's variables into a classes's members. Then, you can easily divide the one huge method into smaller ones that are all contained inside the class and that don't have to pass around all the variables, because those are contained within the same class.