5
\$\begingroup\$

What would be the best way to refactor this :

@busy.each do |b|
  title =  b.title
  @events << {
    :id => b.id,
    :title => title,
    :start => b.busy_start_time(b.start_date,b.start_time),
    :end => b.busy_end_time(b.end_date,b.end_time),
    :allDay => false,
    :recurring => false,
    :color => 'black',
  } 

@events is initialized as an empty array:

@events = []

And this busy statement is the SECOND of two pushes into @events.

I think that I can use map here something like :

@events = @events + @busy.map do |b|
  stuff
end

Because I don't want to overwrite @events, I want to add stuff into it.

But what I'm really searching for is a way to store the entire 'each' into a lambda and say something like :

@events += @busy(&:create_busy_hash) 

And move all the hash creation into the Busy class as create_busy_hash

I'm still grasping lambdas and procs

\$\endgroup\$
4
\$\begingroup\$

I think this is where you are trying to get to:

class Busy
  def event_info
    {
      :id => id,
      :title => title,
      :start => busy_start_time(start_date, start_time),
      :end => busy_end_time(end_date, end_time),
      :allDay => false,
      :recurring => false,
      :color => 'black',
    }
  end
end

@events = @busy_items.map(&:event_info)

Some additional notes:

  • There is no need to initialize @events to an empty array, favor expressions over statements with side-effects (map, select, reduce and so on, instead of each).
  • I renamed the variable to busy_items to emphasize it's a collection.
  • Busy does not sound right for a class name. Classes are usually nouns, not adjectives. What's the nature of this class?
\$\endgroup\$
  • \$\begingroup\$ In this case the "Busy" class represents calendar events where the resource is "Busy", aka unavailable for a period of time that we would like to display on the calendar simply as "busy" much like Google Calendar does when you don't have permission to view details. As such, while traditionally a verb, there is a precedent on calendar applications in Enterprises to simply display an event as 'busy'. Hence, the name. \$\endgroup\$ – notaceo May 29 '14 at 2:38
0
\$\begingroup\$

How about

@events = @busy.map do |b|
  {
    id: b.id,
    title: b.title,
    start: b.busy_start_time(b.start_date,b.start_time),
    end: b.busy_end_time(b.end_date,b.end_time),
    allDay: false,
    recurring: false,
    color: 'black',
  }
end

?

Better still, take an object-oriented approach, something like @events = @busy.map {|b| Event.new(b) }, then define the constructor as appropriate.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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