Request for code review - Weather Data kata

josh.smith · April 20, 2016, 8:17pm

I’m relatively new to Elixir and have been tackling small programming challenges to become better acquainted with the language as I read books about it. Today during lunch I decided to try out the Weather Data “kata” exercise from http://codekata.com/kata/kata04-data-munging/

I got it working, though I don’t know if there is a better way. Here’s my code:

gist.github.com

https://gist.github.com/ijoshsmith/7e2363220f126366c7fb3c0584227a0c

weather.dat

  Dy MxT   MnT   AvT   HDDay  AvDP 1HrP TPcpn WxType PDir AvSp Dir MxS SkyC MxR MnR AvSLP

   1  88    59    74          53.8       0.00 F       280  9.6 270  17  1.6  93 23 1004.5
   2  79    63    71          46.5       0.00         330  8.7 340  23  3.3  70 28 1004.5
   3  77    55    66          39.6       0.00         350  5.0 350   9  2.8  59 24 1016.8
   4  77    59    68          51.1       0.00         110  9.1 130  12  8.6  62 40 1021.1
   5  90    66    78          68.3       0.00 TFH     220  8.3 260  12  6.9  84 55 1014.4
   6  81    61    71          63.7       0.00 RFH     030  6.2 030  13  9.7  93 60 1012.7
   7  73    57    65          53.0       0.00 RF      050  9.5 050  17  5.3  90 48 1021.8
   8  75    54    65          50.0       0.00 FH      160  4.2 150  10  2.6  93 41 1026.3

This file has been truncated. show original

weather.ex

defmodule Weather do

  def print(path) do
    File.stream!(path) 
    |> Stream.map(  &extract_row_from_line/1 ) 
    |> Stream.map(  &drop_unused_columns_from_row/1 )
    |> Enum.map(    &convert_strings_to_integers/1 )
    |> Enum.filter( &is_day?/1 ) 
    |> Enum.reduce( &row_with_smaller_temperature_spread/2 )
    |> print_result

This file has been truncated. show original

If someone with a lot of Elixir experience would please review my code and provide tips and/or suggestions, I’d appreciate it!

Thanks!

Qqwy · June 9, 2016, 7:09am

First and foremost: I find this a very nice and readable solution. Good job!

All right, here are my two cents:

There does not seem to be a reason for using Enum.map and Enum.filter on lines 7 and 8 respectively. I think it is nicer to keep your Stream a Stream for as long as possible.
Maybe your function Weather.print might rather be named Weather.print_day_with_lowest_temperature_spread, although that would again be a very long name… In any case, print is a name that is not self-documenting.
Although documentation is not necessarily part of the Kata, because documentation is a first-class citizen in Elixir, I think it is a good practice to simply always document your public functions in one or two sentences.
There exists the function Enum.min_by/2, which you could use to clean up line 9 (Enum.reduce( &row_with_smaller_temperature_spread/2 )). That could become something like Enum.min_by(&temperature_spread_of_row/1) defp temperature_spread_of_row([_, max, min), do: max - min
Nice job on outlining the functions in the pipeline. That is very readable .
There is no check on columns that have the wrong data format. That is to say, you check if the first column in the row is a non-number (and thus treated as a non-day), but if one of the other columns cannot be parsed as an integer, the program will fail with a not very readable stack trace. (As it will fail when attempting to calculate the average temperature) You might want to add a check for this that raises a nicer exception.
The pipeline on line 22 does not make the function more readable. Simply doing Enum.map(row, fn x -> ... end) is probably better.

So, those were my nitpickings ^^’. In my opinion, you’ve done great on this exercise.

josh.smith · June 9, 2016, 3:46pm

Thank you for taking the time to give me an in-depth review and such detailed feedback!