15
\$\begingroup\$

Just for fun, I want to try to write this Ruby function more succinctly. I imagine it can be done, I'm just not knowledgeable enough with Ruby yet to know how. Any suggestions?

  def tags
    return ['Cancelled'] if cancelled?
    tags = []
    tags << 'Filled' if filled?
    tags << 'In Progress' if in_progress?
    tags
  end 
\$\endgroup\$

migrated from stackoverflow.com Sep 16 '12 at 1:18

This question came from our site for professional and enthusiast programmers.

  • 3
    \$\begingroup\$ @Mark: The question is indeed in the frontier with codereview. But this thing is, a code review is usually broader, on longer code that may have many things to address. Here is just "what's the best way to build an array with conditional elements?". Imo that's ok for SO. \$\endgroup\$ – tokland Sep 15 '12 at 12:31
  • \$\begingroup\$ @MarkThomas, I knew but I forgot. Thanks for the reminder. \$\endgroup\$ – Jason Swett Sep 15 '12 at 12:48
11
\$\begingroup\$
def tags
  if cancelled? then ["Cancelled"]
  else [*("Filled" if filled?), *("In Progress" if in_progress?)]
  end
end

If you design your code so that filled? and in_progress? be false whenever cancelled? is true, then the logic will be much prettier:

def tags
  [
    *("Cancelled" if cancelled?),
    *("Filled" if filled?),
    *("In Progress" if in_progress?),
  ]
end
\$\endgroup\$
  • 1
    \$\begingroup\$ I think this is the best answer, although what I actually ended up doing was a slight variation on your first suggestion. First line: return ['Cancelled'] if cancelled?, second line: [*("Filled" if filled?), *("In Progress" if in_progress?)] \$\endgroup\$ – Jason Swett Sep 15 '12 at 13:22
  • 1
    \$\begingroup\$ The splats (*) here are superfluous and can be removed. \$\endgroup\$ – Andrew Marshall Sep 15 '12 at 13:25
  • 1
    \$\begingroup\$ @AndrewMarshall You would be left with nil if you didn't have the splats. That's the whole point of this answer. \$\endgroup\$ – sawa Sep 15 '12 at 13:28
  • 2
    \$\begingroup\$ @sawa Whoops I thought there was a compact in there (which I would prefer anyway). \$\endgroup\$ – Andrew Marshall Sep 15 '12 at 13:31
  • 1
    \$\begingroup\$ @sawa - I have only seen the splat operator in method definitions like def foo(*args), for example. I never knew it can do this. Can you give more info about it (a link?)? Can we say - if you have conditional values in an array, put a splat before them to eliminate nil values? \$\endgroup\$ – Alex Popov Nov 13 '13 at 22:59
19
\$\begingroup\$

Functional approach:

def tags
  if cancelled?
    ["Cancelled"]
  else
    [("Filled" if filled?), ("In Progress" if in_progress?)].compact
  end
end

Ideas behind the snippet:

  • Don't perform imperative side-effects on arrays (unless performance is a problem, not the case here).
  • Use the pattern: array of "nilable" expressions + compact to build arrays with conditional elements.
  • Minimize the use of inline conditional statements, they make a function harder to understand. They are ok as guards (early exit of functions), but not here. Note that I do use inline ifs, but they are expressions, not statements; more orthodox -but also more verbose- would be (filled? ? "Filled" : nil).
\$\endgroup\$
2
\$\begingroup\$

Not exactly the same, but here's another idea:

def tags
  [ :cancelled?, :filled?, :in_progress? ].map do |m|
    m.to_s if send( m )
  end.compact
end
\$\endgroup\$
  • \$\begingroup\$ LOL, I thought of the same thing. It can get closer in Rails (or by requiring active_support) if you add .titleize to the m.to_s. \$\endgroup\$ – Mark Thomas Sep 15 '12 at 13:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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