Ruby: nested if statements

Question

I was writing some code and it ended up being way too ugly to my liking. Is there anyway that I can refactor it so that I don't use nested if statements?

def hours_occupied(date)
  #assuming date is a valid date object    
  availability = get_work_hours(date)
  focus = "work"

  if availability.nil
    availability = get_family_hours(date)
    focus = "family"

    if availability.nil
      availability = get_friend_hours(date)
      focus = "friends"
    end
  end
end

I know I'll be able to do something like this for availability

availability = get_work_hours(date) || get_family_hours(date) || get_friend_hours(date)

but how do I set the focus variable accordingly?

Can we assume you are not showing the complete method? otherwise it makes no sense to assign those unused variables. If that's the case, add "..." as placeholder at the bottom of the method. — tokland, Jun 19 '13 at 15:22
@tokland: I find myself completely unable to understande the subject and the purpose of his code, which renders me unable to respond. — Boris Stitnicky, Jun 19 '13 at 15:46

score 7 · Answer 1 · answered Jun 19 '13 at 15:01

7

I would do something like the following as it makes it clear that each case is mutually exclusive:

def hours_occupied(date)
  if availability = get_work_hours(date)
    focus = "work"
  elsif availability = get_family_hours(date)
    focus = "family"
  elsif availability = get_friend_hours(date)
    focus = "friends"
  end
end

answered Jun 19 '13 at 15:01

Don Cruickshank

5,641
6
48
48

2

Ruby conditionals are expression, so no need to repeat `focus` 3 times – tokland Jun 19 '13 at 15:26

tokland · Answer 2 · 2017-06-22T22:09:17.467

3

I'd write:

def hours_occupied(date)
  focus = if (availability = get_work_hours(date))
    "work"
  elsif (availability = get_family_hours(date))
    "family"
  elsif (availability = get_friend_hours(date))
    "friends"
  end
  # I guess there is more code here that uses availability and focus.
end

However, I am not sure having different methods for different types is a good idea, it makes code harder to write. A different approach using Enumerable#map_detect:

focus, availability = [:work, :family, :friends].map_detect do |type|
  availability = get_hours(date, type)
  availability ? [type, availability] : nil
end

edited Jun 22 '17 at 22:09

answered Jun 19 '13 at 15:20

tokland

66,169
13
144
170

I never like seeing assignments inside the conditional test. It's too much like C or Perl and gives me nightmares. Thanks a lot. :-) – the Tin Man Jun 19 '13 at 16:14
@theTinMan: You're welcome :-) Some people would say that if the language allows it (for example Python does not), why avoid it? can't you tell a == from a =? But I won't :-) I like this construction because it simplifies the code with less nested expressions. Note that the parens make it more conspicous. – tokland Jun 19 '13 at 16:29

trushkevich · Accepted Answer · 2013-06-20T07:38:36.483

One more way is just to reassign values if there is a need:

def hours_occupied(date)
  availability, focus = get_work_hours(date), "work"
  availability, focus = get_family_hours(date), "family" unless availability
  availability, focus = get_friend_hours(date), "friend" unless availability
end

or using an iterator:

def hours_occupied(date)
  availability = focus = nil
  %w(work family friend).each {|type| availability, focus = self.send(:"get_#{type}_hours", date), type unless availability}
end

score 1 · Answer 4 · answered Jun 19 '13 at 15:42

1

A case when is also an option:

focus = case availability
when get_work_hours(date)
  "work"
when get_family_hours(date)
  "family"
when get_friend_hours(date)
  "friends"
end

answered Jun 19 '13 at 15:42

steenslag

79,051
16
138
171

either this is wrong or `case` works differently than I thought :-) – tokland Jun 19 '13 at 15:56
1

but `availability` must hold the first non-nil *result* of the method calls, it's not known beforehand. – tokland Jun 19 '13 at 16:04
4

Just use `case`, not `case availability` – Stefan Jun 19 '13 at 16:19
Guys, I am a bit puzzled by your comments, check the question, `availability` is not known! or am I missing something? – tokland Jun 19 '13 at 16:28
@tokland that's why you have to use `case` without a parameter: `focus = case when get_work_hours...` – Stefan Jun 19 '13 at 16:31
Yes Stefan, but with an assignment in each `when`, so isn't it absolutely equivalent to an if chain? – tokland Jun 19 '13 at 16:32

Ruby: nested if statements

4 Answers4