HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyrailsMinor

Refactor .each where each is redirected into a hash

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
redirectedeachintowherehashrefactor

Problem

What would be the best way to refactor this :

@busy.each do |b|
  title =  b.title
  @events  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

Solution

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?

Code Snippets

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)

Context

StackExchange Code Review Q#51939, answer score: 4

Revisions (0)

No revisions yet.