Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

I'm switching to Ruby from Java and so, for exercise purposes, I wrote this verbose leap year Ruby program. The comments express my concerns about Ruby best practices. I'd be glad if some Ruby experienced programmer could dispel my doubts. Every hint such as "in Ruby you can achieve this thing better this way" is truly welcomed.

First of all, this is a possible execution:

Pick a starting year:
2000
Now pick an ending year:
2024
From year 2000 to year 2024 there are 7 leap years.
Specifically:
The year 2000 was a leap year,
The year 2004 was a leap year,
The year 2008 was a leap year,
The year 2012 is a leap year,
The year 2016 will be a leap year,
The year 2020 will be a leap year,
The year 2024 will be a leap year.

Please note the verb conjugation and that every line has a comma at the end, except the last one that has a period.

And here is my poor code:

# I just felt bad about calling Time.now.year every time just to know the present year, 
# is this simple memoization a good solution?
def was_is_or_will_be(year)
    @present ||= Time.now.year 
    return 'is'         if year == @present
    return 'was'        if year < @present
    return 'will be'    if year > @present
end

# first of all, let's ask for the years interval
puts 'Pick a starting year:'
starting = gets.chomp.to_i  
puts 'Now pick an ending year:'

# Is this the best "repeat..until" pattern for ruby?
# Because it doesn't seems very clear to me. 
# There is no way to write somethin more explicit like a "do..while"?
while true
    ending   = gets.chomp.to_i   
    if ending > starting
        break
    end
    puts "The ending year must be greater than the starting year,\n please pick a valid ending year:"
end 

# This piece of code seems 'ruby-way' to me... isn't it?
leap_years = []
(starting..ending).each do |year|
    if year%4 == 0 && (year%100 != 0 || year%400 == 0)
        leap_years << year
    end
end

# This is the best way I found to handle this simple pluralization
puts "From year #{starting} to year #{ending} there #{leap_years.count == 1 ? 'is' : 'are'} " + 
            "#{leap_years.count} leap #{leap_years.count == 1 ? 'year' : 'years'}."

# If there are some leap_years in the interval I want a verbose list of them
if leap_years.count > 0
    puts "Specifically:"  

    # Is there a way to achieve the same function (a comma at the end of every line but a period at the end of the last one)
    # Without an 'i' counter? With such as a Java 'hasNext()' equivalent?
  i = 1;
    leap_years.each do |leap_year|
        puts "The year #{leap_year} #{was_is_or_will_be(leap_year)} a leap year#{ i < leap_years.count ? ',' : '.'}"
        i += 1
    end
end

Thank you.

share|improve this question

2 Answers

up vote 4 down vote accepted

Nice effort, :) here are a few comments.

unless you are calling this as part of another class, do not use @member syntax. What you really want is a global.

$present = Time.now.year

Shorter name :), plus it is better to capture it in case statement rather than abuse return :).

def tense(year)
  return case 
      when year < $present ; 'was'
      when year > $present ; 'will be'
      else 'is'
      end
end

def get_years()
  # first of all, let's ask for the years interval
  puts 'Pick a starting year:'
  starting = gets.chomp.to_i  
  puts 'Now pick an ending year:'

Now, what you asked for is possible, for example,

  #begin
  #  ending   = gets.chomp.to_i   
  #end while ending <= starting

However, I dont recommend it.

  while true
    ending   = gets.chomp.to_i   
    break if ending > starting

Use heredocs if you have more than one line to output.

    puts <<-EOF
The ending year must be greater than the starting year,
please pick a valid ending year:
    EOF
  end
  return [starting,ending]
end

Modularize a little bit. get_years is obviously a specific functionality.

starting,ending = get_years()

It is rubyish (and smalltalkish) to use select, inject, collect etc rather than explicit loops where possible.

leap_years = (starting..ending).select do |year|
      year%4 == 0 && (year%100 != 0 || year%400 == 0)
end

Smaller width lines if you can :). It helps, and it looks good. Also make use of case in these cases.

puts "From year #{starting} to year #{ending} there " + case leap_years.count
      when 0 ;  "are no leap years."
      when 1 ;  "is 1 leap year."
      else "are  #{leap_years.count} leap years."
      end

Choose to exit early rather than another indentation level

exit 0 unless leap_years.count > 0

puts "Specifically:"  
puts leap_years.map{|leap_year|
   "The year #{leap_year} #{tense(leap_year)} a leap year"
}.join(",\n") + "."
share|improve this answer
This is moving, thank you! – Duccio Armenise May 30 '12 at 8:20
Just two observations: 1] your inline syntax for the switch case construct doesn't work (am I missing something?) so I changed it for the ordinary multiline one. 2] the last line returns an error because the interpreter tries to add "." to the result of puts (nil) so I changed the last 3 lines adding a 'report' variable this way: "report = leap_years.map ...." and then "puts report". – Duccio Armenise May 30 '12 at 9:37
I have updated the last line. (My fault, I changed it without testing.). What is the error that you get while using inline case? See this example in wiki for a simple use. – rahul May 30 '12 at 14:16
1  
Uhh, if you are using ruby 1.9, please use ';' instead of ':' as case separator. (See updated.) You could also use when ... then ... construct instead. – rahul May 30 '12 at 14:21
And it is also possible to omit the 'return' in your 'tense(year)' method :) – Duccio Armenise May 31 '12 at 9:00
show 2 more comments
up vote 1 down vote

Don't code like below:

while true
    ending   = gets.chomp.to_i   
    if ending > starting
        break
    end
    puts "The ending year must be greater than the starting year,\n please pick a valid ending year:"
end

instead, you should use:

loop do 
  <code goes here>
  break if <condition>
end

see: http://stackoverflow.com/questions/190590/ruby-builtin-do-while and http://blade.nagaokaut.ac.jp/cgi-bin/scat.rb/ruby/ruby-core/6745

share|improve this answer
Thank you very much for this hint about the "loop" construct. I didn't know it. – Duccio Armenise May 30 '12 at 8:22
Uhm, I tried with 'loop do...end' but inside the 'do..end' (the block) the variables are local! So replacing a 'while true' with a 'loop do' doesn't seem to be always proper or convenient. – Duccio Armenise May 30 '12 at 9:36
I just looked it up in my pickaxe book. It is presented as the "Last, and probably least...". Anyway, is good to know about it. One interesting thing to know about Ruby blocks (and that I just learned) is that their inner variables are local by default but if a variable already exists with the same name before the block, then inside the block that variable is local no more! So it's possible tu use loop..do construct in this context provided that the "ending" variable must be defined before it. – Duccio Armenise May 30 '12 at 9:56
1  
I think the problem you mentioned is "shadow variables ( or Variable Shadowing) ", unfortunately, Ruby 1.8 has the bug. you can avoid this by using Ruby1.9. For more details, please refer to <<Metaprogramming Ruby>>, really great book along side with the pickaxe book. – Siwei Shen May 30 '12 at 11:10

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.