How to make this code more Ruby-way?

Question

I am new to Ruby and Rails (switched from Python and Python frameworks). I'm writing a simple dashboard website which displays information about the S.M.A.R.T. state of hard disks. Here I wrote a helper to display a badge in a table cell near the relevant S.M.A.R.T attribute if it's value meets a condition. At first, the helper code was as simple as in Listing 1, but then I decided to draw a summary of all badges for the specific drive, in addition to the badges near individual S.M.A.R.T. attributes in the table. So at first I added a simple method like:

def smart_chk_device(attrs)
    attrs.each { |item| smart_chk_attr(item) }
end

But this approach didn't work and it caused the entire array of attributes to be output to the resulting page. It only started to work when I made it as in Listing 2, but I believe there's something wrong there, and the same thing can be done in a more simple way. Please show me the right, Ruby-way of doing it.

Listing 1:

module HomeHelper
    def smart_chk_attr(attr)
        case attr[:id].to_i
        when 1,197
            content_tag(:span, "Warning", :class => "label label-warning") if attr[:raw].to_i > 0
        when 5,7,10,196,198
            content_tag(:span, "Critical", :class => "label label-important") if attr[:raw].to_i > 0
        end
    end
end

Listing 2 (works, but I don't like it):

module HomeHelper
    def smart_chk_attr(attr)
        case attr[:id].to_i
        when 1,197
            return content_tag(:span, "Warning", :class => "label label-warning") if attr[:raw].to_i > 0
        when 5,7,10,196,198
            return content_tag(:span, "Critical", :class => "label label-important") if attr[:raw].to_i > 0
        else
            return String.new
        end
        return String.new
    end

    def smart_chk_device(attrs)
        output = ""
        attrs.each { |item| output << smart_chk_attr(item) }
        return output.html_safe
    end
end

attrs is an Array of Hashes, where each Hash contains keys :id and :raw with the numeric code of a S.M.A.R.T attribute and its RAW value, both in Strings.

Also, RoR complaints if to remove the last "return String.new" in Listing 2. Why is it so? Doesn't the "case" block all the possible cases, so that control should never reach the end of the function?

Answer 1

I believe this would behave in the same way, and is much shorter:

module HomeHelper

  def smart_chk_attr(attr)
    return '' unless attr[:raw].to_i > 0
    case attr[:id].to_i
      when 1,197
        content_tag(:span, "Warning", :class => "label label-warning")
      when 5,7,10,196,198
        content_tag(:span, "Critical", :class => "label label-important")
      else ''
    end
  end

  def smart_chk_device(attrs)
    attrs.map { |item| smart_chk_attr(item) }.join.html_safe
  end

end

Ruby methods return the value of the last expression, so get rid of the explicit returns all over that method. Also, DRY: Don't Repeat Yourself (the attr[:raw] check). In this case, I replaced those with a guard clause at the start of the method. Short-circuiting guard clauses are a matter of taste, but I like them and you'll see them in a lot of Ruby code.

Answer 2

Your method smart_chk_attr(attr) has an extra return at the end, it will never get run.

The each is an enumerator, when it finishes going through each item that you supplied it returns the the original object passed into it, not the modified stuff inside.

If you use collect you will get an array with your modified objects. If you are wanting a string output you can use join to put them into a string. Join will also take an option for how to join your items.

def smart_chk_device(attrs)
  attrs.collect{ |item| smart_chk_attr(item) }.join.html_safe
end