3
\$\begingroup\$

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.

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

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
\$\endgroup\$
2
  • \$\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 Jul 27 '16 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 '16 at 20:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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