Why are else statements discouraged in Ruby?

Question

I was looking for a Ruby code quality tool the other day, and I came across the pelusa gem, which looks interesting. One of the things it checks for is the number of else statements used in a given Ruby file.

My question is, why are these bad? I understand that if/else statements often add a great deal of complexity (and I get that the goal is to reduce code complexity) but how can a method that checks two cases be written without an else?

To recap, I have two questions:

1) Is there a reason other than reducing code complexity that else statements could be avoided?

2) Here's a sample method from the app I'm working on that uses an else statement. How would you write this without one? The only option I could think of would be a ternary statement, but there's enough logic in here that I think a ternary statement would actually be more complex and harder to read.

def deliver_email_verification_instructions
  if Rails.env.test? || Rails.env.development?
    deliver_email_verification_instructions!
  else
    delay.deliver_email_verification_instructions!
  end
end

If you wrote this with a ternary operator, it would be:

def deliver_email_verification_instructions
  (Rails.env.test? || Rails.env.development?) ? deliver_email_verification_instructions! : delay.deliver_email_verification_instructions!
end

Is that right? If so, isn't that way harder to read? Doesn't an else statement help break this up? Is there another, better, else-less way to write this that I'm not thinking of?

I guess I'm looking for stylistic considerations here.

Answer 1

Let me begin by saying that there isn't really anything wrong with your code, and generally you should be aware that whatever a code quality tool tells you might be complete nonsense, because it lacks the context to evaluate what you are actually doing.

But back to the code. If there was a class that had exactly one method where the snippet

if Rails.env.test? || Rails.env.development?
    # Do stuff
else
    # Do other stuff
end

occurred, that would be completely fine (there are always different approaches to a given thing, but you need not worry about that, even if programmers will hate you for not arguing with them about it :D).

Now comes the tricky part. People are lazy as hell, and thusly code snippets like the one above are easy targets for copy/paste coding (this is why people will argue that one should avoid them in the first place, because if you expand a class later you are more likely to just copy and paste stuff than to actually refactor it).

Let's look at your code snippet as an example. I'm basically proposing the same thing as @Mik_Die, however his example is equally prone to be copy/pasted as yours. Therefore, would should be done (IMO) is this:

class Foo
  def initialize
    @target = (Rails.env.test? || Rails.env.development?) ? self : delay
  end

  def deliver_email_verification_instructions
    @target.deliver_email_verification_instructions!
  end
end

This might not be applicable to your app as is, but I hope you get the idea, which is: Don't repeat yourself. Ever. Every time you repeat yourself, not only are you making your code less maintainable, but as a result also more prone to errors in the future, because one or even 99/100 occurrences of whatever you've copied and pasted might be changed, but the one remaining occurrence is what causes the @disasterOfEpicProportions in the end :)

---

Another point that I've forgotten was brought up by @RayToal (thanks :), which is that if/else constructs are often used in combination with boolean input parameters, resulting in constructs such as this one (actual code from a project I have to maintain):

class String
  def uc(only_first=false)
    if only_first
      capitalize
    else
      upcase
    end
  end
end

Let us ignore the obvious method naming and monkey patching issues here, and focus on the if/else construct, which is used to give the uc method two different behaviors depending on the parameter only_first. Such code is a violation of the Single Responsibility Principle, because your method is doing more than one thing, which is why you should've written two methods in the first place.

Answer 2

def deliver_email_verification_instructions
  subj = (Rails.env.test? || Rails.env.development?) ? self : delay
  subj.deliver_email_verification_instructions!
end