14
\$\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\$
2
  • 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, 2012 at 12:31
  • \$\begingroup\$ @MarkThomas, I knew but I forgot. Thanks for the reminder. \$\endgroup\$
    – Jason Swett
    Sep 15, 2012 at 12:48

3 Answers 3

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\$
5
  • 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, 2012 at 13:22
  • 1
    \$\begingroup\$ The splats (*) here are superfluous and can be removed. \$\endgroup\$ Sep 15, 2012 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, 2012 at 13:28
  • 2
    \$\begingroup\$ @sawa Whoops I thought there was a compact in there (which I would prefer anyway). \$\endgroup\$ Sep 15, 2012 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\$ Nov 13, 2013 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\$
1
  • \$\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\$ Sep 15, 2012 at 13:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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