F677fa685a2cfe8aff31f161062db3d3

I'm pretty sure theres a better way use this if else logic, I need to make sure the @events and @recurrents objects are not nil before iteration.

1
2
3
4
5
6
7
8
9
10
11
12
if @events != nil
  @events.each do |event|
    if @recurrents != nil
      @recurrents.each do |recurrent|
        r = Runt::DIWeek.new(recurrent.weekday)
        event_hold << event unless r.include?(event.date)
      end
    else
      event_hold << event
    end
  end
end

Refactorings

No refactoring yet !

8fb47582197b2d50cd7d297841c8cc87

chrismo

August 11, 2008, August 11, 2008 01:30, permalink

No rating. Login to rate!

You could start the method with...

1
2
3
return if @events.nil? || @recurrents.nil?

@events.each do ...
Cd2382441bbe2ec68e3bf36aea0ca2ea

The Real Adam

August 11, 2008, August 11, 2008 02:10, permalink

No rating. Login to rate!

Not sure how you're fetching @events and @recurrents, but you could just guarantee they're not nil. One way is to hide them behind an accessor, like below. It looks like you need the conditional for recurrents, so I left that alone.

Also, I'm not quite sure what you're up to here, but you could possibly replace the appends to event_hold with a call to inject.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
        def events
          @events || []
        end

        def recurrents
          @recurrents || []
        end

          @events.each do |event|
            unless recurrents.empty?
              recurrents.each do |recurrent|
                r = Runt::DIWeek.new(recurrent.weekday)
                event_hold << event unless r.include?(event.date)              
              end
            else
              event_hold << event
            end
          end
Avatar

leemic

August 12, 2008, August 12, 2008 06:09, permalink

No rating. Login to rate!

Make @recurrent check first then @event loop.

However, I don't know if this is correct logic. Are you trying to say you want to hold 'event' when it does not overlap with Runt::DIWeek date? Because you are doing two loops, you are going to select 'event' when you should not.

For example, your event dates are [ '2008-08-01' , '2008-08-16' ]. Let's say your '@recurrents' are [ '2008-08-01' , '2008-08-02' ]. Since you are doing double loops, you end up select ['2008-08-01']

Also depending on the size of @events and @recurrents - you may be creating a lot of Runt::DIWeek unnecessary. Create it once and reuse them.

1
2
3
4
5
6
7
8
9
10
11
12

if @events != nil
 
  if ( @recurrent == nil )
     event_hold = Array.new( @events )
  else
     restricted_dates = @recurrents.collect { |recurrent| Runt::DIWeek.new( recurrent.weekday ) }.flatten.unique
     event_hold = @events.select { |event|  ! restricted_dates.include?(event.date) }
  end

end
8484df61a1434e91cb088f53cc089f18

Adam

August 12, 2008, August 12, 2008 15:02, permalink

No rating. Login to rate! 
1
2
3
4
5
event_hold = Array(@recurrents).inject(Array(@events)) do |events,recurrent|
  events.reject! do |event|
    Runt::DIWeek.new(reucrrent.weekday).include?(event.date)
  end
end

Your refactoring





Format Copy from initial code

or Cancel