Is “call and return” a pattern or an antipattern?

Question

Imagine to have this code:

class Foo:
  def __init__(self, active):
    self.active = active
  def doAction(self):
    if not self.active:
      return
      # do something

f=Foo(false)
f.doAction() # does nothing

This is a nice code; I actually have (not in Python) a global active variable called "dosomething" and a routine called "something," where the first thing happening inside the "something" routine is if not dosomething return.

An alternative implementation would call to a routine that always performs an action, and the flag is checked at invocation time, as in the following code:

class Foo:
  def doAction(self):
    # do something

doaction = False

f=Foo()
if doaction:
  f.doAction()

What is your opinion on this? I personally find that the first solution violates the least surprise principle: The caller is invoking an action which is never performed in response to a status which has been set somewhere else, but from the code it looks like the action is performed.

Would you consider it a total pattern, a total antipattern, or just an option with no strong opinion for or against it?

Answer 1

I believe both have their place; depending on where the on/off logic should reside.

I would use the first more encapsulated case if the check should be local to Foo, e.g. if it was a connection state for the internals of Foo for example.

The second external check case makes more sense if the caller correctly knows the state of doAction. If the caller were of class Bar, and sometimes wanted to use Foo for some purpose and sometimes do something else perhaps.

Answer 2

While I commonly see call and return used throughout my colleague's code (and mine), I wouldn't recommended it in this case (we use it to implement the fail fast property). If possible, try to apply the State pattern.

In this case, active and inactive would be valid states upon which the implementation of doAction(...) changes. This should clear up the code and make it more maintainable. Additionally, you get the benefit of state transitions, e.g., informObservers(boolean active).

Answer 3

I believe that this is a useful pattern, provided that you have an assertion (or debug log message) before it returns. When the application is deployed, the assertion should be off and thus the method should return harmlessly without crashing. On the other hand you should see it during development and can refactor your code so that the call is not made in the first place.

Answer 4

The relevant principle is in my opinion: Tell, Don't Ask

The question is: is self.active something within the domain of Foo.

If it is in Foo's domain, and you go for the second solution, you would violate encapsulation by imposing knowledge of it to the caller, and should be avoided. A good example would be a Customer, that is considered active when he has payed it's bills.

If it is not in Foo's domain, one should refactor, so that active it is not a member variable of Foo.

Answer 5

I wouldn't dogmatically say this is a pattern or an antipattern. I think this pattern has a place in "good" code. It depends on the context. For example, I wouldn't want to see this kind of thing in a method called WriteToDatabase(). When I call that method, I expect something to be written to the database. However, if there was a method called OpenDatabaseConnection(), I'd be okay with that method not doing anything if the connection was already opened. That way different callers or different threads or whatever can always call it just to be safe, but they don't have to check for "openness" before they make the call.

Answer 6

Your first example is valid OO, and could be used for many things like being sure a connection is open, etc. I'm not sure if it has a name.

The second example is ok. Not really a pattern in my mind, just a condition check before performing an action. (Well, I guess Sequence, Selection, Iteration is a pattern.)

Both methods are useful in different circumstances.