Rubyize this : 2nd edition

Woooah… my first post in 3 weeks. Don’t worry, it will get back to the normal soon. I have a new job and, well, you know what it is… the stress and all the new stuff to learn. Anyway, my contract will expire by the end of August so things will run smoothly again pretty soon.
Yay! Another edition of Rubyize this. For those who don’t know what it is, you just have to rewrite the code snippet into some solid and idiomatic Ruby. This is a scientific process that is also called “Rewriting a code snippet into some solid and idiomatic Ruby”.
This week we dive a bit more into the Rails world. Take for grant that the following code is located inside a view :

<%songs = Song.find(:all)
for each song in songs%>
  

<%=song.title[0..0].upcase + song.title[1..song.title.length]%>

<% rating = song.rating if rating.nil? rating = 0 end if rating > 0 for i in 1..rating%> star <% end else%> This song hasn't been rated yet <%end end%>

Yikes… how ugly.
You can also specify some of your changes in plain english if you want (e.g. I would put this in a controller, I would use a partial to…, etc)
IMPORTANT NOTE : Since you cannot indent your code in the comments, you can try to put some dots or underscores instead of spaces… that way your code should remain fairly easy to read. Also, DON’T use <% and %> because it will get stripped out by wordpress! Use something else… like just the percentage sign “%”. I’m sorry about all these quirks… I absolutely have to make the comments “Ruby friendly”. For now, I strongly suggest that you copy your snippet in your clipboard BEFORE sending it. That way, if some of your code get stripped out, you won’t be that pissed off and you probably won’t try to kill me. That’s a win-win situation.

21 thoughts on “Rubyize this : 2nd edition

  1. I’d do something like:
    Controller:
    @songs = Song.find(:all)
    View:
    h2%= @song.title.titleize %/h2
    %= show_rating(@song.rating) %
    Helper:
    def show_rating rate
    ..if rate && rate.nonzero?
    ….rate.times { image_tag ‘star.gif’, :alt => ‘star’ }
    ..else
    ….”This song hasn’t been rated yet”
    ..end
    end

  2. Make that:
    def show_rating rate
    ..if rate && rate.nonzero?
    ….(1..rate).collect { image_tag ‘star.gif’, :alt => ‘star’ }
    ..else
    ….”This song hasn’t been rated yet”
    ..end

  3. Well first I don’t like to set variables in the viewn so I would move the find statement to the controller:
    @songs = Song.find(:all)
    While there’s no problem with a for loop, the syntax is a bit off, so I’d change that to:
    Next .capitalize would be better for putting the title’s first letter in uppercase:
    The rating statement could definetely do with a bit of trimming:
    ” * song.rating : “This song hasn’t been rated yet” %>
    would be the way I’d clean it up, and I’d probably move it to a helper too.
    Anyway that’s my take on it 😉
    Note: Just in case the formatting on this gets messed up, I’ve made a Pastie at http://pastie.caboo.se/83477

  4. Edit: Oops, soory, forgot about the not using Erb tags so most I’ve my post did indeed get obliterated. Jan largely beat me too it anyway though.

  5. Hi!
    I wrote it in haml, think it might be easier to read (still need .. for indent though!)
    ###############################
    -for each song in Song.find(:all)
    ..%h2=song.title.first.upcase
    ..(song.rating||0).times do
    ….%img{:src=>’star.gif‘, :alt>=’star’}/
    ..=”This song hasn’t been rated yet” unless song.rating
    ###############################

  6. Did you really want to display songtitle’s first letter (title[0..0].upcase), or did you want to titleize it (song.title.titleize)?

  7. # songs_controller.rb
    class SongsController 0
    ……for i in 1..rating
    ……..image_tag(“star.gif”, :alt => “star”)
    ……end
    ….else
    …..”This song hasn’t been rated yet”
    ….end
    ..end
    end
    # index.rhtml
    % @songs.each do |song| -%
    ..%= song.title.capitalize %
    ..%= rating_stars(song.rating) %
    % end -%

  8. Pretty much a rehash of what has been said with a few refinements:
    # Controller
    @songs = Song.find(:all)
    # Helper
    def show_rating(rating)
    . if rating && rating > 0
    . . (1..rating).each { image_tag ‘star.gif’, :alt => ‘*’ }
    . else
    . . “This song has not been rated”
    . end
    end
    # View (HAML)
    – @songs.each do |song|
    . %h2= h(song.titleize)
    . = show_rating(song.rating)

  9. @Eric… darn, you’re right! I indeed wanted to display the title with the first letter capitalized… my bad. I’m going to make the ugly code uglier again.


  10. # Controller
    def show
    @songs = Song.find(:all)
    end
    # Helper
    def song_rating(song)
    if song.rating.to_i > 0
    image_tag('star.gif') * song.rating.to_i
    else
    "This song hasn’t been rated yet"
    end
    end
    # View
    >% @songs.each do |song| %<
    >h2<>%=h song.title.capitalize %<>/h2<
    >%= song_rating song %<
    >% end %<
    # Me
    self.drink! :coffey

  11. Fun! OK, here’s mine. Not much different from everyone else’s, but I thought I’d throw it in the ring.
    First, in the controller:
    @songs = Song.find(:all)
    Second, in the migration that created songs, I’d set the default for rating to 1 and make rating an int. This gets rid of the need to check for nil in the rating. I’d think about validating for this in the model as well.
    Here’s the view:
    % @songs.each do |song| %
    h2%= song.title.titleize %/h2
    %= song.rating > 0 ? image_tag(‘star.gif’) * rating : “This song hasn’t been rated yet” %
    % end %
    I tend to use the trinary operator a lot, so I find that readable. Others may differ 🙂
    Scott
    Oh yeah, if you use image tag like that, you need to have star.gif in the public/images directory. This seems more railsy to me. If you really want it in public, then I think that calling it like image_tag(‘/star.gif’) will do the trick.

  12. Wow. I find it kinda disheartening that only one out of thirteen solutions broke away from the PHP/JS for/in mindset and went with the much more native Ruby Array#each. The Ruby compiler has to translate those “foreign” phrases into Ruby’s Array#each every time you use them. 🙁 I really, really hate that so many “Rubyists” continue to baby-feed their students this ugly, clunky, fallback methodology. It’s usually a telltale sign [to me] that someone doesn’t really know their Ruby. 🙁

  13. I don’t really think its really an issue of not knowing Ruby. I just think the syntax of for x in y is nicer than y.each do |x|. While I take the point about it being marginally slower, this is true (often to a larger extent) of a large number of other widely used, convenient methods, like link_to or to_proc. It’s just one of the downsides of Ruby I’m afraid.

  14. @ Alex: Neither link_to nor to_proc are Ruby methods themselves but Railsisms. I stand by my statement that a preference for the for/in methodology usually belies a programmer who hasn’t come to fully understand Ruby. For/in is syntactic sugar for Array#each, put there for programmers who can”t or won’t let go of programming habits they learned in other languages. Array#each is not only marginally faster but more importantly… it’s more _Ruby_. And by learning to think in Ruby [instead of translating your old programming languages’ idioms to Ruby] one will no doubt see a marked increase in the efficiency of his/her code.

  15. Thanks to RSL for pointing this out.
    It actually was the first time I ever used “for … in ….” instead of @songs.each{|song| }
    I just saw it in previous code, and gave it a try!
    But in every Ruby tutorial I saw, .each has been used.
    I didn’t know “for … in …” was syntactic sugar for Array#each though…
    Do you have any other examples of “Rubyer” code?
    do..end vs {}
    “a”

  16. @ Eric: All the Ruby tutorials use Array#each [because they’re teaching Ruby] but the Rails tutorials [and even the books, shame shame!] use for/in to attract programmers from other languages. It’s one of my pet peeves as it conditions the new Rails/Ruby programmer to keep thinking in his/her old programming language. This neglect to understand Ruby and Rails trickles usually ends up creating what I “lovingly” refer to as FrankenModels, with code and structures that use Ruby but look more like a PHP program.
    As far as do..end or braces, they’re both very Ruby. The difference tends to be mostly stylistic, with braces being used more for short, one-line code blocks and do..end for blocks that take more code. Don’t let this fool you into thinking they’re the same though. When using variable assignment and the return value from a block, do…end will sometimes raise an error as it doesn’t bind as tightly as braces do and the assignment can’t properly determine what’s going on. This doesn’t happen a lot but when it does, you’ll be glad you learned that braces bind tighter.

Leave a Reply to Alex Cancel reply

Your email address will not be published. Required fields are marked *