Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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 
share|improve this question

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

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

3  
@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. –  tokland Sep 15 '12 at 12:31
    
@MarkThomas, I knew but I forgot. Thanks for the reminder. –  Jason Swett Sep 15 '12 at 12:48
    
so it was finally moved... well, I think it was perfectly ok for SO, but whatever. –  tokland Sep 16 '12 at 9:08

3 Answers 3

up vote 10 down vote accepted
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
share|improve this answer
1  
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?)] –  Jason Swett Sep 15 '12 at 13:22
1  
The splats (*) here are superfluous and can be removed. –  Andrew Marshall Sep 15 '12 at 13:25
1  
@AndrewMarshall You would be left with nil if you didn't have the splats. That's the whole point of this answer. –  sawa Sep 15 '12 at 13:28
2  
@sawa Whoops I thought there was a compact in there (which I would prefer anyway). –  Andrew Marshall Sep 15 '12 at 13:31
1  
@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? –  Alex Popov Nov 13 '13 at 22:59

Functional approach:

def tags
  if cancelled?
    ["Cancelled"]
  else
    [("Filled" if filled?), ("In Progress" if in_progress?)]
  end.compact
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).
share|improve this answer

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
share|improve this answer
    
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. –  Mark Thomas Sep 15 '12 at 13:56

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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