4
\$\begingroup\$

I've noticed a tendency towards two styles of conditionally executing methods:

  1. Conditional before the call

    def foo(thing)
      puts thing
    end
    
    if thing
      foo(thing)
    end
    
  2. Conditional within the call

    def foo(thing)
      return unless thing
      puts thing
    end
    
    foo(thing)
    

Pros of 1:

  • Method is concise and easier to read
  • Method can assume input is valid
  • Obvious what's going on in calling code

Pros of 2:

  • Caller doesn't need to check condition every time the method is invoked
  • Calling code is cleaner and easier to read
  • Conditional is centralised in one location and not duplicated (DRY!)

Anecdotally, I've noticed that most people tend to lean towards the first style. Lately I've found my self leaning towards the second style, for the pros I've given above.

Thoughts? Any other advantages/disadvantages I've missed? Any other styles?

\$\endgroup\$

closed as off-topic by RubberDuck, Jamal Jul 1 '14 at 20:36

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions must involve real code that you own or maintain. Questions seeking an explanation of someone else's code are off-topic. Pseudocode, hypothetical code, or stub code should be replaced by a concrete example." – RubberDuck, Jamal
If this question can be reworded to fit the rules in the help center, please edit the question.

10
\$\begingroup\$

I think that one of the pros you listed for style 2 - "Conditional is centralized in one location and not duplicated" is sufficient all by itself to tip the scale to style 2.

Non-duplication of code is a very powerful idea, particularly in a system under development. There is always the possibility that something will change. Duplicated code is very fragile, because of the possibility of overlooking one or more instances. Even if you don't overlook an instance, changing multiple instances of duplicated code is always harder than changing one single instance of non-duplicated code.

\$\endgroup\$
  • \$\begingroup\$ Er, did you mean the opposite here: "Even if you don't overlook an instance, changing multiple instances of duplicated code is always easier than changing one single instance of non-duplicated code." \$\endgroup\$ – u2622 Aug 17 '12 at 20:49
  • \$\begingroup\$ And yes, that's one of the main reasons I'm leaning towards doing it this way. Somehow it feels a bit odd though. I thought it might be a code smell, but I wasn't sure. \$\endgroup\$ – u2622 Aug 17 '12 at 20:50
  • \$\begingroup\$ Fixed the reversed logic. Duh. \$\endgroup\$ – Donald.McLean Aug 17 '12 at 20:53
5
\$\begingroup\$

Option 2 is the proper direction, with some reservations. Option 2 is cleaner, implements defensive programming, and always implement the desired logic (dry).

The reservations have to do with the actual concrete implementation. A few critical questions: Is "if thing" always required or is it dependent on where/when you need to check it (conditional)? With that, if the answer was conditional...at what level is that condition applicable (globally, per type of thing, per situation)?

Even after getting all those questions answered, it is still better to implement those conditional variances into the foo() method or it's overload(s),...option 2. The rare case is where the conditional applicability is too random.

Just a tip, follow readability. It usually steers you in the right direction.

Also, you had noted as one of the pros for option 1 is that it is concise and easier to read. I think if you were to start to utilize the foo() method hundreds or thousands of times, you would most likely scratch that pro.

Hope this helps.

\$\endgroup\$
1
\$\begingroup\$

The two options are not really two options as they are not mutually exclusive.

You should prefer two for things that are tied closely with foo and need to be checked everytime that foo is done, you should use one where it is not always applicable to foo, but it is under these circumstances.

IOW if all code paths include a check for x immediately before calling foo, and foo would break or take an incorrect action if x was incorrect, then put the check in foo, if on the ther hand, it is only checked 4 out of 5 times, then obviously it doesn't belong in foo.

An additional consideration is whether foo is being called to return a result or to take an action. If to return a result, then you may be just changing one check at the call site for another, in which case it depends upon the check -- which makes more sense.

\$\endgroup\$
  • \$\begingroup\$ Thanks - you are right that they are not exclusive! \$\endgroup\$ – u2622 Aug 30 '12 at 20:30

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