Rails Proper Use of Unless with And Operator

Question

I have a Rails 3.2.18 app where I'm trying to send Twilio notifications to a medic's phone on the update action using:

 unless @call.call_status == "close" && @call.unit_ids.empty?

If a call status is set to close then that means the call is closed and no notifications need to be sent out via ActionMailer or my send_sms private method. Also if the unit_ids is empty (array of units) no mail or sms alerts should go out either.

Here's what happens. If The call is open and I have no units assigned (unit_ids is empty) and I update the call, it will bypass the action mailer but it still tries to fire off send_sms and raises a nilclass exception for @call.units.first.incharge since the medic/incharge does not exist in the array. Now if I nest another unless statement within the first one (see second example working, it will not fire mailer or send_sms if the call_status == close and unit_ids.empty?

calls_controller.rb (not working)

def update
    parse_times!
    @call = Call.find(params[:id])

    respond_to do |format|
      if @call.update_attributes(params[:call])
        unless (@call.call_status == "close" && @call.unit_ids.empty?)
          send_sms

        @call.send_mail(:update_call)

        end
        format.html { redirect_to @call, notice: "Call #{@call.incident_number} was successfully updated.".html_safe }
        format.json { head :no_content }
      else
        format.html { render action: "edit" }
        format.json { render json: @call.errors, status: :unprocessable_entity }
      end
    end
  end

private
def send_sms
    account_sid = 'AACCCCCC'
    auth_token = 'ATttttt'
    @client = Twilio::REST::Client.new account_sid, auth_token
    @client.account.messages.create(
       :from => '2814084444',
       :to => @call.units.first.incharge.medic_phone,
       :body => "incident_number #{@call.incident_number} patient name #{@call.patient_name}"
      )
    @client.account.messages.create(
      :from => '2814084444',
       :to => @call.units.first.attendant.medic_phone,
       :body => "incident_number #{@call.incident_number} patient name #{@call.patient_name}"
      )
  end
end

calls_controller.rb (working)

def update
    parse_times!
    @call = Call.find(params[:id])

    respond_to do |format|
      if @call.update_attributes(params[:call])
        unless @call.call_status == "close"
          unless  @call.unit_ids.empty?
          send_sms
        end

        @call.send_mail(:update_call)

        end
        format.html { redirect_to @call, notice: "Call #{@call.incident_number} was successfully updated.".html_safe }
        format.json { head :no_content }
      else
        format.html { render action: "edit" }
        format.json { render json: @call.errors, status: :unprocessable_entity }
      end
    end
  end

  def send_sms
    account_sid = 'ACCCCCC'
    auth_token = 'atttt'
    @client = Twilio::REST::Client.new account_sid, auth_token
    @client.account.messages.create(
       :from => '2814084444',
       :to => @call.units.first.incharge.medic_phone,
       :body => "incident_number #{@call.incident_number} patient name #{@call.patient_name}"
      )
    @client.account.messages.create(
      :from => '2814084444',
       :to => @call.units.first.attendant.medic_phone,
       :body => "incident_number #{@call.incident_number} patient name #{@call.patient_name}"
      )
  end

Is nesting the unless statements the proper way to do it or can I somehow combine the && operator with unless. Maybe my syntax is wrong in the first (non working) example. I'm obviously missing something here, so any help that is offered would be appreciated.

Update: 08/03/14-08:03

It seems that if I use the || operator I do not raise a nilclass exception and the mailer and send_sms fires unless the call status is close or the call unit_ids is empty.

 unless (@call.call_status == "close" || @call.unit_ids.empty?)
          send_sms
          @call.send_mail(:update_call)
        end

Does this make sense or should I try to use the && operator?

Answer 1

As you have correctly observed, nesting two conditionals is equivalent to combining the conditions with &&. Since unless ... is the same as if ! ... ("if not") you could also write:

if !(@call.call_status == "close") && !(@call.unit_ids.empty?)
  # ...
end

De Morgan's Law teaches us that !a && !b is the same as !(a || b):

if !(@call.call_status == "close" || @call.unit_ids.empty?)
  # ...
end

Of course, we can substitute back the if ! with an unless:

unless @call.call_status == "close" || @call.unit_ids.empty?
  # ...
end

Although I would rather advise you to use if with complex conditionals, because that is generally easier to read and understand. We start again with eliminating the unless:

if !(@call.call_status == "close") && !(@call.unit_ids.empty?)
  # ...
end

Then, we reformulate the parts left and right of && so that the ! is no longer necessary. In this case, we can change == to !=. We can also change empty? to any?, but that would be problematic when the array contains only nil and/or false values. If you can be sure that this never happens, you can use x.any? instead of ! x.empty? as well:

if @call.call_status != "close" && !@call.unit_ids.empty?
  # ...
end

# If you are 100% sure that @call.unit_ids _never_ contains nil or false values,
# you can use x.any? instead of !x.empty?

if @call.call_status != "close" && @call.unit_ids.any?
  # ...
end