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.

I'm often presented with these two options. From a design point of view, which of these is subjectively better, and why?

Option A

class foo {
  private boolean bar() {
    //Stuff
  }

  private void A() {
     if(bar())
       B();
  }

  private void B() {
      // more stuff
  }
}

Option B

class foo {
  private boolean bar() {
    //Stuff
  }

  private void A() {
      B();
  }

  private void B() {
     if(!bar())
        return;

     // more stuff
  }
}

Let's assume that code duplication isn't an issue here, and the division is only to separate, logically, the code for better readability/understanding (A, B, and bar have meaningful names in context). Is it more readable to have the reader know that B might not do anything when looking at just A? Or is the encapsulation of state better put into B? Obviously, this can be refactored to separate B and bar() into a separate class entirely, which separates concerns even more, but assuming mostly shared state, which is better?

share|improve this question
2  
You'll get more useful responses if you accept that either might be right in some circumstances, and ask for examples of appropriate circumstances –  Kate Gregory Mar 23 '11 at 22:55
    
In Option B, method A is totally pointless. –  whatsisname Jun 22 '13 at 19:34

5 Answers 5

up vote 5 down vote accepted

The important bit is who owns bar? If it's unclear, consider if B can live up to it's method name considering the value of Bar. If so, then make it part of B. If not, then make it part of A.

In a real world scenario, you'll generally find methods that fulfill a design responsibility, and helper methods that fulfill implementation steps. Generally, the decision of whether to call an implementation method resides with the API method - which is generally the public method.

public class Control {
    public bool IsVisible;
    public void Render() {
       if (this.IsVisible) {
          this.PrepareForRender();
       }
    }
    private void PrepareForRender() {
       // just do it. Don't check IsVisible
    }
 }

It's possible that you have two different API methods calling each other. In that case, I'd suggest falling back on the semantics of why doesn't B run when Bar is false? Is it because B is unnecessary - then B should decide.

 public class List {
    public int Count;
    public void SetTo(object[] values) {
       this.Clear(); // need to clear values; don't really care how
    }
    public void Clear() {
       // Count = 0 is just a shortcut to fulfilling Clear's responsibility
       if (this.Count == 0) return; 
    }
 }

Is it because B is incorrect? Then A should decide.

public class File {
    public bool IsBinary;
    public void Write() {
       if (this.IsBinary) {
          this.OpenBinaryStream();
       } else {
          this.OpenTextStream();
       }     
    }
    private void OpenBinaryStream() {
    }
    private void OpenTextStream() {
    }
 }

Because it's actively harmful? A should decide, and B should throw an exception:

public class File {
    public bool UseSecureDelete;
    public void Delete() {
       if (this.UseSecureDelete) {
          this.ZeroBytes();
       }
    }
    private void ZeroBytes() {
       if (!this.UseSecureDelete) throw new InvalidOperationException();
    }
 }
share|improve this answer

I strongly disagree with @jmquigley.

Whatever "performance gain" is in-play here is minuscule and negligible.

The "if" condition should reside in the member in which it's logically (or even semantically) a part of.

Since you haven't clued us in to what the actual object or member functions are, only you can make this decision.

share|improve this answer
1  
+1 agree, I was wrong. –  jmq Mar 24 '11 at 4:04
    
I've clarified the question a bit, which I hope helps. In short, my question is exactly what you state, what helps determine the logical place? Is 'possibly not executing' a concern of the caller or the called? –  Neal Tibrewala Mar 24 '11 at 6:58

It depends on whether foo() and B() lie on the same level of abstraction.

If they do, then Option A is preferable.

Otherwise (foo() is an implementation detail of B() and therefore lower than B()) Option B is better.

So, the guiding principle here is: don't mix levels of abstraction.

share|improve this answer

Shortest code wins. You don't want to have multiple appearances of:

 if(!bar.contains(baz))
   B(baz, stuff);

If you NEVER want to add something to bar that is already present, the second implementation is clearly superior.

However, in almost all modern languages, you could make bar a set, and then say something like this:

if (bar.add(baz))
  // more stuff
share|improve this answer
    
+1 good point. I was looking at it a different way, but I like your analysis (I should have been thinking DRY). –  jmq Mar 24 '11 at 4:02
    
Thanks for the answer. I've clarified the question a bit. Duplication isn't an issue, this is an isolated algorithm that's divided only for readability, not re-use, yet. However, I agree a re-use scenario might force the decision one way or another. –  Neal Tibrewala Mar 24 '11 at 6:59

I suggest you consult with the mythical rubber duck of the "Rubber duck debugging" fame. Don't be fooled by his debugging powers because he is also adept at figuring out design issues.

share|improve this answer
    
I asked the design Teddy Bear, and he was stumped too, but thanks for the suggestion! –  Neal Tibrewala Mar 24 '11 at 7:00

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.