mcordell/noaa — pull request #1 by randycoulman (2014-06-27 07:02:32+00:00)
Title: Refactor Timelayout and its specs per post on Ruby Rogues Parley.

Timelayout: I moved all of the XML extraction logic to the class and added
an initialize method that just takes the layout_key and layout. The class
essentially becomes a factory for creating instances of Timelayout from an
XML node, but the instances know nothing about XML nodes.

With the new initialize method, it becomes easier to create instances of
Timelayout for testing elsewhere, since it is essentially a simple value
object - no need to mock or stub it.

I also simplified the layout_key extraction; it wasn't clear from the original
code and specs if well-formed input should ever be missing the key - it
seemed like that might just be for testing convenience; it would be easy to
change that back if necessary; there should also be a spec added for the
missing layout_key case.

I'd be tempted to turn the time ranges into a real object as well; using the
(possibly nested) arrays seems like primitive obsession.

Timelayout spec:

I eliminated what I felt to be a number of redundant and repetitive specs.
IMO, the respond_to specs are satisfied implicitly by the other specs; if
the object didn't respond to those messages, the other specs wouldn't
run at all.

I try not to test that a response is of a certain class; I prefer to rely on
duck typing instead.

By testing against the expected array contents, the need for multiple
expects is eliminated.

It might be worth adding specs for mis-formed input, like a missing
layout_key, or a mismatched number of start and end times. It might
also be worth adding a spec for input with no times specified.

I'm not sure I'm happy with the XML data setup, but it does eliminate some
duplication in the timestamp strings.

Overall, this is just how I'd approach refactoring the code you posted. Feel
free to accept, reject, or ask for changes in this PR, and thanks for the fun
exercise!