9
\$\begingroup\$

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
\$\endgroup\$
0

5 Answers 5

11
\$\begingroup\$
  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.

\$\endgroup\$
4
  • \$\begingroup\$ divmod + I didn't know about that headers options, nice! +1 \$\endgroup\$
    – tokland
    Commented Jul 3, 2014 at 7:54
  • \$\begingroup\$ 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. \$\endgroup\$
    – pgreen2
    Commented Jul 3, 2014 at 11:20
  • 1
    \$\begingroup\$ 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. \$\endgroup\$
    – Nzall
    Commented Jul 3, 2014 at 13:55
  • 1
    \$\begingroup\$ @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 :) \$\endgroup\$
    – Flambino
    Commented Jul 3, 2014 at 18:04
6
\$\begingroup\$

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
\$\endgroup\$
1
  • \$\begingroup\$ Shame on me for you using variables like f. \$\endgroup\$
    – pgreen2
    Commented Jul 3, 2014 at 11:21
2
\$\begingroup\$

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
}
\$\endgroup\$
2
  • \$\begingroup\$ 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. \$\endgroup\$
    – pgreen2
    Commented Jul 3, 2014 at 3:47
  • \$\begingroup\$ 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. \$\endgroup\$
    – Marc-Andre
    Commented Jul 3, 2014 at 11:02
1
\$\begingroup\$

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
\$\endgroup\$
0
\$\begingroup\$

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.

\$\endgroup\$
4
  • \$\begingroup\$ I totally agree, but that is not within my control. \$\endgroup\$
    – pgreen2
    Commented Jul 3, 2014 at 14:32
  • \$\begingroup\$ This is an excellent comment, but unfortunately not an answer. This should be moved up as a comment on the question. \$\endgroup\$ Commented Jul 3, 2014 at 15:53
  • \$\begingroup\$ 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? \$\endgroup\$
    – Nzall
    Commented Jul 3, 2014 at 15:55
  • \$\begingroup\$ 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. \$\endgroup\$
    – rolfl
    Commented Jul 3, 2014 at 16:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.