Skip to content
This repository was archived by the owner on Jul 24, 2020. It is now read-only.

refactor complex method available? in ReservationValidations #343

Merged
merged 7 commits into from
Jul 7, 2014

Conversation

squidgetx
Copy link
Contributor

No description provided.

@ebmaher
Copy link
Contributor Author

ebmaher commented Jun 27, 2013

unless my tests are way off, this method is at least partially broken, so that needs to be addressed as well

@squidgetx
Copy link
Contributor

The instance method for EquipmentModel .num_available(date_range) used to call available_count(date) for every date in the date range. available_count would query the database for all the reservations on that date and subtract from the total available equipment items, causing O(n) behavior with respect to db queries. I just added a couple scopes to allow for a couple (O(1)) queries at the start of the method and adjusted everything accordingly.

With a clean db generated from rake db:seed minimal=true development takes 1.2s on my setup to register a cart date change with a range of 1 day, and 3.7s to register a change with a range of 24 days. This branch clocks in at 1.2s and 1.7s respectively

@@ -55,6 +55,8 @@ class Reservation < ActiveRecord::Base
scope :missed_requests, lambda {where("approval_status = ? and start_date < ?", 'requested', Time.now.midnight.utc)}

scope :for_reserver, lambda { |reserver| where(reserver_id: reserver) }
scope :reserved_in_date_range, lambda { |start_date, end_date|
where("start_date < ? and due_date > ?",end_date, start_date) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add checks to filter so that it only returns approval_status == 'auto' or 'approved'. your new code already filters that out via the not_returned scope, but I think it would be good to filter here also in case anyone ever calls reserved_in_date_range by itself. another thing I should have considered before, in #297, is setting up a scope for just approval_status == 'auto' or 'approved', and then chaining it onto the other scopes like we're already doing with .recent for example.

@dgoerger
Copy link
Contributor

dgoerger commented Jul 7, 2014

looks good to me!

squidgetx added a commit that referenced this pull request Jul 7, 2014
refactor complex method available? in ReservationValidations
@squidgetx squidgetx merged commit d21201b into development Jul 7, 2014
@orenyk
Copy link
Contributor

orenyk commented Jul 7, 2014

Nice, looks great!

Oren Kanner
PhD Candidate, Mechanical Engineering
GRAB Lab, Yale University

On Mon, Jul 7, 2014 at 12:26 PM, Sylvan Zheng [email protected]
wrote:

Merged #343 #343.


Reply to this email directly or view it on GitHub
#343 (comment).

@dgoerger dgoerger mentioned this pull request Jul 7, 2014
@dgoerger dgoerger deleted the 343_refactor_available branch July 10, 2014 16:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants