Method flags as arguments or as member variables?

Question

I think the title "Method flags as arguments or as member variables?" may be suboptimal, but as I'm missing any better terminology atm., here goes:

I'm currently trying to get my head around the problem of whether flags for a given class (private) method should be passed as function arguments or via member variable and/or whether there is some pattern or name that covers this aspect and/or whether this hints at some other design problems.

By example (language could be C++, Java, C#, doesn't really matter IMHO):

class Thingamajig {
  private ResultType DoInternalStuff(FlagType calcSelect) {
    ResultType res;
    for (... some loop condition ...) {
      ...
      if (calcSelect == typeA) {
        ...
      } else if (calcSelect == typeX) {
        ...
      } else if ...
    }
    ...
    return res;
  }

  private void InteralStuffInvoker(FlagType calcSelect) {
    ...
    DoInternalStuff(calcSelect);
    ...
  }

  public void DoThisStuff() {
    ... some code ...
    InternalStuffInvoker(typeA);
    ... some more code ...
  }

  public ResultType DoThatStuff() {
    ... some code ...
    ResultType x = DoInternalStuff(typeX);
    ... some more code ... further process x ...
    return x;
  }
}

What we see above is that the method InternalStuffInvoker takes an argument that is not used inside this function at all but is only forwarded to the other private method DoInternalStuff. (Where DoInternalStuffwill be used privately at other places in this class, e.g. in the DoThatStuff (public) method.)

An alternative solution would be to add a member variable that carries this information:

class Thingamajig {
  private ResultType DoInternalStuff() {
    ResultType res;
    for (... some loop condition ...) {
      ...
      if (m_calcSelect == typeA) {
        ...
      } ...
    }
    ...
    return res;
  }

  private void InteralStuffInvoker() {
    ...
    DoInternalStuff();
    ...
  }

  public void DoThisStuff() {
    ... some code ...
    m_calcSelect = typeA;
    InternalStuffInvoker();
    ... some more code ...
  }

  public ResultType DoThatStuff() {
    ... some code ...
    m_calcSelect = typeX;
    ResultType x = DoInternalStuff();
    ... some more code ... further process x ...
    return x;
  }
}

Especially for deep call chains where the selector-flag for the inner method is selected outside, using a member variable can make the intermediate functions cleaner, as they don't need to carry a pass-through parameter.

On the other hand, this member variable isn't really representing any object state (as it's neither set nor available outside), but is really a hidden additional argument for the "inner" private method.

What are the pros and cons of each approach?

Answer 1

On the other hand, this member variable isn't really representing any object state (as it's neither set nor available outside), but is really a hidden additional argument for the "inner" private method.

That reinforces my gut feeling upon reading your question: try to keep this data as local as possible, i.e. don't add it to the object state. This reduces the state space of your class, making your code simpler to understand, test and maintain. Not to mention that avoiding shared state makes your class more threadsafe too - this may or may not be a concern to you right now, but it may make a huge difference in the future.

Of course, if you need to pass around such flags excessively, it may become a nuisance. In this case, you may consider

• creating parameter objects to group related parameters together into a single object, and/or
• encapsulating this state and the associated logic into a separate family of strategies. The viability of this depends on the language: in Java, you need to define a distinct interface and a (possibly anonymous) implementation class per each strategy, whereas in C++ or C# you may use function pointers, delegates or lambdas, which simplifies the code considerably.

Answer 2

I would definitely vote for the "argument" option - my reasons:

1. Synchronization - what if the method is called from multiple threads at the same time? By passing the flags as an argument, you won't have to sychronize access to these flags.

2. Testability - Let's imagine you are writing an unit test for you class. How you will test the internal state of the member? What if your code throws an exception and the member will end in an unexpected state?

3. Separation of concerns - By adding the flags to your method arguments, it is clear that no other method is supposed to use it. You won't use it by mistake in some other method.

Answer 3

The first alternative seems OK to me. A function is called with a parameter, and it does something that depends on that parameter. Even if you just forward the parameter to another function, you do it expecting that the result will somehow depends on the parameter.

The second alternative feels like using a global variable, just within the class scope instead of application scope. But it's the same problem... you must read carefully the whole class code to determine who and when is reading and writing those values.

Therefore the first alternative wins.

If you would use too many parameters (read: two or more) in the first alternative, which all just get passed together to the next function, you can consider putting them into one object (make it an inner class, if it's not relevant for the rest of the application) and using this object as a parameter.

Answer 4

I want to say "neither".

First off, consider that you already know what you're going to be doing when you're calling DoInternalStuff. The flag says as much. So now, instead of just going ahead and doing it, you're farting around calling a function that now has to decide what to do.

If you want to tell the function what to do, then tell it what to do. You can pass an actual function you want to run for each iteration as a C# delegate (or a C++ function object, or a Java runnable or the like).

In C#:

class Thingamajig {
  private delegate WhateverType Behavior(ArgsType args);

  private ResultType DoInternalStuff(Behavior behavior) {
    ResultType res;
    for (... some loop condition ...) {
      ...
      var subResult = behavior(someArgs);
      ...
    }
    ...
    return res;
  }

  private void InteralStuffInvoker(Behavior behavior) {
    ...
    DoInternalStuff(behavior);
    ...
  }

  public void DoThisStuff() {
    ... some code ...
    InternalStuffInvoker(doThisStuffPerIteration);
    ... some more code ...
  }

  public ResultType DoThatStuff() {
    ... some code ...
    ResultType x = DoInternalStuff(doThatStuffPerIteration);
    ... some more code ... further process x ...
    return x;
  }
}

You still end up with the argument passing all over the place, but you get rid of most of the if/else crap in DoInternalStuff. You could also store the delegate as a field if you wanted to.

As for whether to pass the todo-thingie around or store it...if you have to choose one, then pass it around. Storing it is likely to bite you in the ass later, as you've pretty much neglected thread safety. Consider that if you intend to do anything with threads, now you have to lock for the entire duration of the loop, or someone can switch todo's right out from under you. Whereas if you pass it, it's less likely to get trashed...and if you still need to lock or something, you can do it for part of one iteration instead of the whole loop.

Frankly, it's good that it's annoying. There's probably a much simpler way to do what you want to do, which i couldn't really advise you on since the code is obviously not the real thing. The best answer varies per case.