← Back to Upcase

Any idea how to refactor this method?

(Alejandro Pereira ) #1
def booked_days                                                              
  days = []       
  reservations.each do |reservation|                                                                                     
    ( (reservation.check_in)..(reservation.check_out - 1.day) ).each { |d| days << d }                                   

(Geoff Harcourt) #2

Hi @alexpereira,

First off, any time you’re setting up an empty array and then shoving stuff into it from an enumeration, you’ve found an excellent candidate for #inject (or perhaps #each_with_object)! I think #inject isn’t used enough given how powerful it is.

I’d also consider adding a method to your Reservation class that returns the date range, as this current method is in charge of doing a lot of different things and has to have knowledge of reservations. Maybe each reservation can return its own booked days and then this class you have (which might be for a customer?) can map and flatten all the booked days from the different reservations?

(Alejandro Pereira ) #3

Thanks for your help dude, the #each_with_object do the job, and add a method on Reservations, result much better idea.

def booked_days 
	reservations.each_with_object([]) do |reservation, days|
		reservation.booked_days.each { |day| days << day }

(Geoff Harcourt) #4

Shorter, cleaner methods for the evolutionary win. Already looks better!

(Derek Prior) #5

This is more idiomatically an inject. Also, I think you can get rid of the each…

def booked_days
  reservations.inject([]) do |days, reservation|
    days += reservations.booked_days

There’s one key difference between inject and each_with_object, but it’s not a problem in this case. inject accumulates based on the return value of the block, whereas each_with_object assumes you will mutate the object if you want it changed. I tend to prefer ruby methods over those from active support where possible and it works well here.

(Geoff Harcourt) #6

I spend so little time doing non-Rails coding that I hadn’t even realized that #each_with_object is an AS method.