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.

The point of the script below is to read in the 10th column which is the total number of minutes a runner took to finish the marathon and convert it into form of "hours:minutes:seconds". Is there a better way?

require 'csv'

File.open('bostonmarathon-2012-results.txt', 'w') {  |f| 
  skip = true
  CSV.foreach("bostonmarathon-2012-results.csv") do |row|
    if skip
      skip = false
    else
      total_seconds = row[10].to_f * 60
      seconds = total_seconds % 60
      minutes = (total_seconds / 60) % 60
      hours = total_seconds / (60 * 60)

      f.puts(format("%02d:%02d:%02d", hours, minutes, seconds))
    end
  end
}

Time starts as in total minutes in decimal format: 142.93

Example Data:

division,name,city,gender,age,official,bib,overall,state,genderdiv,net,country
4        / 26,"Soejima, Masazumi",Fukuoka City,M,41,83.45,W1,4        / 35,,4        / 26,83.45,JPN
14       / 4335,"Gebremariam, Gebregziabher",Tigray,M,27,142.93,2,14       / 21616,,14       / 12621,142.93,ETH
6        / 26,"Van Dyk, Ernst F.",Paarl,M,39,84.38,W2,6        / 35,,6        / 26,84.38,RSA
2        / 26,"Fearnley, Kurt H.",Hamilton,M,31,81.65,W3,2        / 35,,2        / 26,81.65,AUS
3        / 26,"Hokinoue, Kota",Iizuka,M,38,83.43,W4,3        / 35,,3        / 26,83.43,JPN
share|improve this question
    
What does the data look like? Is minutes with decimal places? –  nhgrif Jul 3 at 2:17
    
Yes, the minutes is in decimal format. –  pgreen2 Jul 3 at 2:22
1  
    
@Jamal, thank you –  pgreen2 Jul 3 at 17:04

5 Answers 5

up vote 10 down vote accepted
  1. Use do...end for multiline blocks. You're already doing it for the innermost block, but not the File.open block for some reason

  2. CSV.foreach accepts a hash of options. One of them is :headers which you can set to true or :first_row. This will

    • skip the header row when iterating (so you can skip skip and the if..else-branching), and
    • let you refer to a column by its header text instead of an opaque numerical index.

For the time-formatting you can use #divmod to get both quotient and remainder of a division in one go. Add some array destructuring, and you get this:

hours, remainder = total_seconds.divmod(60**2)
minutes, seconds = remainder.divmod(60)

Pretty clean, I think.


Alternatively, here's a hacky(!) idea. You could just treat the seconds as a regular unix timestamp, and do something like

Time.at(total_seconds).strftime("%k:%M:%S")

The assumption here is of course that no one took more than 24 hours to complete the marathon. If they did, the hour would of course roll over. It seems like an ok assumption in this particular case, but it's obviously not something that'd work for any context. It's kinda clever, but it's definitely a hack.

share|improve this answer
    
divmod + I didn't know about that headers options, nice! +1 –  tokland Jul 3 at 7:54
    
This was exactly what I was looking for, +1 for :header and +1 for divmod. I agree with you about strftime but if a value were to exceed 24 hours, it would be touch to detect, so I prefer not to do that. –  pgreen2 Jul 3 at 11:20
1  
You can walk a marathon at normal walking speed (5 km/h) in about 8.5 hours. Unless a runner is somehow unable to finish the race and completes it later, like happened in 1912, a marathon that lasts over 24 hours is extremely unlikely. And even if that somehow happened, there are 86,400 seconds in a day. divide the total seconds by 86,400 for the days and parse the remainder normally. check, division, remainder, alternate represenation: 4 lines of code. but it would be rare. a 24 hour marathon would be at a speed of 1.75 km/h. You can do that while crawling. –  Nate Kerkhofs Jul 3 at 13:55
1  
@NateKerkhofs True, the strftime trick should cover 99.9999% of cases, and it's easy to check if it won't. But then you'd have to add that extra check, branch the code, and add the usual div/mod solution to handle that eventuality. And the div/mod solution already works for 100% of cases, so why bother with the first strftime solution at all? From a software engineering perspective that's just gross :) I mentioned it here because I think it's kinda clever, but that doesn't make it less of a hack :) –  Flambino Jul 3 at 18:04

Some notes on your code:

  • Use do/end for multi-line blocks.
  • skip = true. That usage of flags is (IMHO) not acceptable, at least not in Ruby. Whenever possible write iterations without state.
  • f. Try to use more declarative names. results, output, something like that.

I'd write:

require 'csv'

File.open('bostonmarathon-2012-results.txt', 'w') do |results| 
  rows = CSV.enum_for(:foreach, "bostonmarathon-2012-results.csv")

  rows.drop(1).each do |row|
    total_seconds = row[10].to_f * 60
    seconds = total_seconds % 60
    minutes = (total_seconds / 60) % 60
    hours = total_seconds / (60 * 60)

    results.puts("%02d:%02d:%02d" % [hours, minutes, seconds])
  end
end
share|improve this answer
1  
+1 for the insisting on state-less iterations and descriptive names –  Flambino Jul 3 at 9:24
    
Shame on me for you using variables like f. –  pgreen2 Jul 3 at 11:21

I'm really not a ruby programmer, but using a boolean skip to skip the header seems really a weird way to me. That solution add an if to your that will only be needed once a variable use only once too.

I'm sure there is a way to skip header of the file without that awful condition.

If you do really need you're if condition, I would suggest that you put the most used block of code first. Something that would look like :

require 'csv'

File.open('bostonmarathon-2012-results.txt', 'w') {  |f| 
  parse = false
  CSV.foreach("bostonmarathon-2012-results.csv") do |row|
    if parse
      total_seconds = row[10].to_f * 60
      seconds = total_seconds % 60
      minutes = (total_seconds / 60) % 60
      hours = total_seconds / (60 * 60)
    else
      parse = true

      f.puts(format("%02d:%02d:%02d", hours, minutes, seconds))
    end
  end
}
share|improve this answer
    
The boolean is only controlling the skipping of the first row. For everything except the first row, I need to parse the row and output it. It looks like your example will only try to print one line that I think will fail do to hours, minutes, and seconds not being set. –  pgreen2 Jul 3 at 3:47
    
Well the first time it's false so it will go in the else and set it to true. After that it will always go in the if part (after the first row), but like I said you should not do that. –  Marc-Andre Jul 3 at 11:02

Instead of tracking the first line myself with a boolean, adding :headers => :first_row does it for me. A secondary effect of using headers is that I can reference columns by name instead of index (row['net'] vs row[10]).

By using the method of divmod, the breaking down of total_seconds becomes a lot more readable.

Lastly, I used do .. end instead of { } because the body was multi lined.

Final code after changes:

require 'csv'

File.open('bostonmarathon-2012-results.txt', 'w') do |output| 
  CSV.foreach("bostonmarathon-2012-results.csv",{:headers => :first_row}) do |row|
    total_seconds = row['net'].to_f * 60

    hours, remainder = total_seconds.divmod(60**2)
    minutes, seconds = remainder.divmod(60)

    output.puts(format("%02d:%02d:%02d", hours, minutes, seconds))
  end
end
share|improve this answer

Not directly related to your code, but if you have any control at all over the data you get, I'd recommend finding a way to provide that time in seconds. You lose a bit of precision when going from base 60 to base 100 during the seconds to decimal conversion, which you can't really get back when converting from base 100 to base 60 again. I doubt that the runner results for a race are provided in minutes, because you lose up to a 5th of a second in precision that way.

share|improve this answer
    
I totally agree, but that is not within my control. –  pgreen2 Jul 3 at 14:32
    
This is an excellent comment, but unfortunately not an answer. This should be moved up as a comment on the question. –  Mark Thomas Jul 3 at 15:53
    
I originally wrote it as a comment as well, but since this also is intended as an improvement on the code, I thought it would suit as an answer equally well. It's probably borderline between comment and answer, since it's a way to improve the code indirectly. Are there guidelines specific to when to comment and when to answer? –  Nate Kerkhofs Jul 3 at 15:55
    
I agree that this is borderline, but it does offer some review of the code, so I am electing to leave it as an answer. –  rolfl Jul 3 at 16:09

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.