Code review: experienced developers edition

[Cory Kaufman-Schofield]

Hi everyone,

At the end-of-the-year drinkup in December, someone mentioned they'd like to see some Ruby code from experienced developers, and have the group discuss it on the mailing list. In order to kick this off, I made a gist of some code I wrote recently:

https://gist.github.com/allspiritseve/4ed3c87875d90a17521b

In a nutshell, this is a single method that takes an iCal daily recurrence string and rewrites the start and/or end times based on the current sunrise / sunset times in a specific location. The method is part of a Condition class that is used as criteria for triggering an event. Here's an example use case:

If I arrive home between sunrise and sunset in Ann Arbor, MI, open all of my living room shades.

I chose this example not because it's necessarily ideal code, but because I think it has some interesting design decisions that would make for good discussion.

You are all welcome to comment on the code; feel free to ask questions or highlight specific sections that you like / dislike. Alternative approaches / philosophies are encouraged! Please use Github for comments related to the code, as it is much better suited for this purpose than email.

* * *

I'd like to see at least two other experienced Ruby developers volunteer some code. Who's game?

Cory

[Tom Smyth]

I'd be up for this. Perhaps we should kick Cory's around for a bit first?

--
You received this message because you are subscribed to the Google Groups "Ann Arbor Ruby Brigade" group.
To unsubscribe from this group and stop receiving emails from it, send an email to a2rb+uns...@googlegroups.com.
To post to this group, send email to a2...@googlegroups.com.
Visit this group at http://groups.google.com/group/a2rb.
For more options, visit https://groups.google.com/d/optout.

--

Tom Smyth

Worker-Owner, Sassafras Tech Collective

Specializing in innovative, usable tech for social change 

sassafras.coop · @sassafrastech

Resident, Touchstone Cohousing

touchstonecohousing.org

[Tom Smyth]

Nice work! A few questions:

• Line 4: Why read_attribute? There appear to be two things named 'recurrence': an object that responds to :next_occurrence, and a string. Am I missing something? Perhaps they should have different names?
• Lines 9 and 21: Is this effectively defining methods inside of methods? Why not make them regular private methods?
• Lines 30-35 (timezone) seem like they could be in their own method. Any reason why not?
• When does this method get called? Only on creation of the object, or at any time? It seems like it would be only on creation if it's going to sit around and serve as a condition for triggering an event.

On 26 January 2015 at 15:50, Cory Kaufman-Schofield <co...@corykaufman.com> wrote:

--

You received this message because you are subscribed to the Google Groups "Ann Arbor Ruby Brigade" group.
To unsubscribe from this group and stop receiving emails from it, send an email to a2rb+uns...@googlegroups.com.
To post to this group, send email to a2...@googlegroups.com.
Visit this group at http://groups.google.com/group/a2rb.
For more options, visit https://groups.google.com/d/optout.

[Cory Kaufman-Schofield]

Tom,

Responses to your questions here:

https://gist.github.com/allspiritseve/4ed3c87875d90a17521b#comment-1381159

Cory