Getting timeslots using a single query

Question

I want to get latest time slots for 2 cases based on doorman true for false based on logged in user.

Cases are:

If doorman is true, the query will be the same
if doorman is false, then I need to add a parameter in the query for doorman that its is false so give me only records having none in them

So, it's the same query almost that will work for both cases with little modification.

def self.latest_pickup_date current_zone,doorman
    if doorman
      latest_timeslot = Timeslot.where(dropoff_slots: '-1', zone_id: current_zone).order(:slot_date).last 
      // it is working fine
    else
      latest_timeslot = Timeslot.where(dropoff_slots: '-1', zone_id: current_zone, doorman_type: "none").order(:slot_date).last 
      // it also does the same job but need to need check if doorman is none, so need refactoring my queries to use one query for both cases
    end
    latest_timeslot.nil? ? Date.current : latest_timeslot.slot_date
  end

I would like to refactor my code and the query in a way to use the approach of DRY.

I do not want to write these queries twice in both cases. I need a better solution using code practices. Or if I am doing this right way, you can advice as well. Plus, I need good professional code practices and code refactoring as well.

Your question says you want to "get all timeslots", but your code appears to fetch the latest timeslot. Please clarify what you intend to accomplish. Also retitle your question according to the purpose of the code — see How to Ask. — 200_success
– 200_success, Commented Jul 24, 2016 at 15:17
I didnt ask for "get all timeslots". I simple want to refactory my code to make it more better. I need suggestions not only an answer. — LearningROR
– LearningROR, Commented Jul 24, 2016 at 15:23
Your first sentence says "Scenario is, I want to get all timeslots for 2 cases." — 200_success
– 200_success, Commented Jul 24, 2016 at 15:26
you are right in saying this but my intention is a refactoring. There is nothing error in my code. I am asking for better approach. — LearningROR
– LearningROR, Commented Jul 24, 2016 at 15:27
In any case, please edit your question and the title to clarify what the code does. — 200_success
– 200_success, Commented Jul 24, 2016 at 15:29

tokland · Accepted Answer · 2016-07-27 20:38:55Z

1

Some notes:

Without knowing the internals of the app, the use of some hardcoded values like dropoff_slits=-1 or doorman_type="none" look a bit suspicious.
Keep the maximum line length to 80 (100 at most) chars.
Write parens in method signature: def self.latest_pickup_date(current_zone, doorman).
latest_timeslot.nil? ? Date.current : latest_timeslot.slot_date. Use Object#try instead: latest_timeslot.try(:slot_date) || Date.current.

There are many valid ways of DRYing this method. That's what I'd write:

def self.latest_pickup_date(current_zone, doorman)
  base_timeslots = Timeslot.where(dropoff_slots: '-1', zone_id: current_zone)
  timeslots = doorman ? base_timeslots : base_timeslots.where(doorman_type: "none")
  timeslots.order(:slot_date).last.try(:slot_date) || Date.current
end

edited Jul 27, 2016 at 20:38

answered Jul 26, 2016 at 21:14

tokland

11.2k1 gold badge21 silver badges26 bronze badges

\$\begingroup\$ What do you mean by: Keep the line length limit to 80/100 chars. and Write parens in method signature.? Could you please explain? \$\endgroup\$

LearningROR
– LearningROR

2016-07-27 17:48:03 +00:00
Commented Jul 27, 2016 at 17:48
\$\begingroup\$ Added comment on the second. I think the first is clear: don't write lines over 80/100 chars, they are unreadable. \$\endgroup\$

tokland
– tokland

2016-07-27 20:39:14 +00:00
Commented Jul 27, 2016 at 20:39

Add a comment |

Stack Exchange Network

Getting timeslots using a single query

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Getting timeslots using a single query

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions