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
      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
    latest_timeslot.nil? ? Date.current : latest_timeslot.slot_date

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.

  • \$\begingroup\$ 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. \$\endgroup\$ Jul 24, 2016 at 15:17
  • \$\begingroup\$ 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. \$\endgroup\$ Jul 24, 2016 at 15:23
  • \$\begingroup\$ Your first sentence says "Scenario is, I want to get all timeslots for 2 cases." \$\endgroup\$ Jul 24, 2016 at 15:26
  • \$\begingroup\$ 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. \$\endgroup\$ Jul 24, 2016 at 15:27
  • \$\begingroup\$ In any case, please edit your question and the title to clarify what the code does. \$\endgroup\$ Jul 24, 2016 at 15:29

1 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
  • \$\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\$ 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
    Jul 27, 2016 at 20:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.