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:

1. If doorman is true, the query will be the same
2. 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.

• 200_success: 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.
• LearningROR: 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.
• 200_success: Your first sentence says "Scenario is, I want to get all timeslots for 2 cases."
• LearningROR: 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.
• 200_success: In any case, please edit your question and the title to clarify what the code does.

Answer 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

• LearningROR: What do you mean by: Keep the line length limit to 80/100 chars. and Write parens in method signature.? Could you please explain?
• tokland: Added comment on the second. I think the first is clear: don't write lines over 80/100 chars, they are unreadable.