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
– tokland, Commented Jun 19, 2013 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
– Boris Stitnicky, Commented Jun 19, 2013 at 15:46

Don Cruickshank · Accepted Answer · 2013-06-19 15:01:13Z

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, 2013 at 15:01

Don Cruickshank

6,0086 gold badges53 silver badges50 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

tokland Over a year ago

Ruby conditionals are expression, so no need to repeat focus 3 times

tokland · Accepted Answer · 2017-06-22 22:09:17Z

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, 2017 at 22:09

answered Jun 19, 2013 at 15:20

tokland

68.2k13 gold badges151 silver badges174 bronze badges

2 Comments

the Tin Man Over a year ago

I never like seeing assignments inside the conditional test. It's too much like C or Perl and gives me nightmares. Thanks a lot. :-)

tokland Over a year ago

@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.

trushkevich · Accepted Answer · 2013-06-20 07:38:36Z

2

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

edited Jun 20, 2013 at 7:38

answered Jun 19, 2013 at 18:40

trushkevich

2,6771 gold badge30 silver badges38 bronze badges

Comments

steenslag · Accepted Answer · 2013-06-19 15:42:37Z

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, 2013 at 15:42

steenslag

80.2k16 gold badges144 silver badges174 bronze badges

6 Comments

tokland Over a year ago

either this is wrong or case works differently than I thought :-)

tokland Over a year ago

but availability must hold the first non-nil result of the method calls, it's not known beforehand.

Stefan Over a year ago

Just use case, not case availability

tokland Over a year ago

Guys, I am a bit puzzled by your comments, check the question, availability is not known! or am I missing something?

Stefan Over a year ago

@tokland that's why you have to use case without a parameter: focus = case when get_work_hours...

|

Collectives™ on Stack Overflow

Ruby: nested if statements

4 Answers 4

1 Comment

2 Comments

Comments

6 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

1 Comment

2 Comments

Comments

6 Comments

Your Answer

Sign up or log in

Post as a guest

Related