Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question
4  
I always seem to struggle with the abstract code. If I saw a specific real example (beyond foo, bar, spam..) of how this is consumed, then I could suggest something. This approach may be appropriate, or it may be over-engineered. The only way to suggest something simpler is to see a specific example of usage. Is OOP the best way to model what is going on? Are you truly taking advantage of OOP by creating many hierarchies, or are you creating classes because the language forces you? Can we assume that you are using Python and nothing else? If not, then what languages are you talking about here? –  Job Sep 3 '11 at 14:09
add comment

6 Answers

up vote 5 down vote accepted

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.

share|improve this answer
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
    
careful, it's not a check. it's an enabling of the very feature that class is designed to provide. –  Stefano Borini Jan 26 '11 at 17:11
    
That would violate encapsulation, if it is enabling a feature. The variable should be a field in the object. –  Michael K Jan 26 '11 at 17:56
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
add comment

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).

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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