current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 1 down vote favorite

Ruby doesn't have a pass method to 'do nothing' the way Python does - if I want a ternary statement to do something on true, but nothing on false (as in the code below) is it bad practice in Ruby to fake it this way?

beverage_links = []

beverage_info.each do |bvg|
   self.is_menu_item?(bvg) ? beverage_links << bvg["href"] : "pass"
end
share|improve this question
1  
What's wrong with a normal conditional expression? beverage_links << bvg["href"] if self.is_menu_item?(bvg). Note also: self is the implicit receiver, no need to explicitly state it. Predicate methods are indicated by a question mark in their name, there is no need to name them is_something. And the whole thing can be better expressed using higher-level iteration methods: beverage_info.select(&method(:menu_item?)).map {|bvg| bvg['href'] }. –  Jörg W Mittag Jan 20 at 12:25
1  
@200_success, this is real code from a gem I'm working on to get drink information from Starbucks' website (github.com/RSid/starbucksstats). I just cut out the parts of the class that were irrelevant to my question. I can add in more context if that's preferred, but it wouldn't have any impact on what I'm asking. –  RSid Jan 20 at 14:31
2  
if is not a statement in Ruby. It's an expression. In fact, in Ruby, everything is an expression, there are no statements. In C, the conditional operator is needed because if is a statement, and the operators are expressions, so when you need an expression, you cannot use if, you have to use the conditional operator. But in Ruby, you never need the conditional operator, you can always use if, because it is already an expression. The conditional has weird precedence which will sometimes force you to add parentheses, which the if expression doesn't have. –  Jörg W Mittag Jan 20 at 14:42
1  
@200_success I have an answer for this question but you have closed it, even though it does not appear to need more context –  Devon Parsons Feb 11 at 14:06
1  
I encourage you to post all of starbucksstats.rb as a separate question. The use of a module is questionable, and there is a lot of repeated code. –  200_success♦ Feb 11 at 23:26

1 Answer 1

up vote 3 down vote accepted

What it looks like you really want is:

beverage_links = beverage_info.select{|bvg| self.is_menu_item?(bvg)}.map{|bvg|bvg["href"]}

This might be a bit confusing, so to break it down

beverage_info.select { |bvg| self.is_menu_item?( bvg ) }

This returns an array of items in beverage_info that return true* for is_menu_item?

  .map { |bvg| bvg["href"] }

This translates each of the items in the array returned in the previous into their "href" value. Finally you assign that to beverage_links.

If beverage_info is an array of beverages, I recommend you rename it to beverages. It is obvious that if you have a beverage, you have the info of that beverage.

If beverages are a custom class, give it the method is_menu_item? because nothing else will use that (unless you have, say, burgers that are also menu items. In that case, both Beverages and Burgers should extend some Food class that has the is_menu_item? method). The way you have it now, it doesn't look like you need to be using the self keyword, but it's hard to tell without context.

* Actually, it returns all items that return ANYTHING that isn't false or nil for is_menu_item?

share|improve this answer
    
Thanks! This was a helpful and complete answer. I'm guessing the general consensus is that faking a 'pass' is, in addition to being the wrong solution here, usually a sign of not writing ruby-idiomatic code and to be re-examined. –  RSid Feb 11 at 21:17

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.