5
\$\begingroup\$

I am getting a date range from form inputs - the "from" month, day, and year, and the "to" month, day, and year, and then running a report on data for within that date range. I want to first check that the dates are valid and if not display an error indicating dates are not valid, then run the report only if valid. I want to

  1. make sure the dates themselves are valid, i.e., no blank inputs, and nothing like February 30th, then
  2. make sure the "from" date is before the "to" date.

I am having trouble doing this in a clean way with repeating myself a bunch. Here's the long way. Please help refactor with assigning to variables but only if valid.

unless Date.valid_date?(params[:from][:year].to_i, 
 params[:from][:month].to_i, 
 params[:from][:day].to_i) &&
 Date.valid_date?(params[:to][:year].to_i, 
 params[:to][:month].to_i, 
 params[:to][:day].to_i)
 redirect_to :back, alert: "Please provide valid date values." and return
end
unless Date.new(params[:from][:year].to_i, 
 params[:from][:month].to_i, 
 params[:from][:day].to_i) <
 Date.new(params[:to][:year].to_i, 
 params[:to][:month].to_i, 
 params[:to][:day].to_i)
 redirect_to :back, alert: "Please provide valid date values." and return
end
from_date = Date.new(params[:from][:year].to_i, 
 params[:from][:month].to_i, 
 params[:from][:day].to_i)
to_date = Date.new(params[:to][:year].to_i, 
 params[:to][:month].to_i, 
 params[:to][:day].to_i)
run_report(from_date, to_date)
200_success
145k22 gold badges190 silver badges478 bronze badges
asked Nov 9, 2015 at 16:51
\$\endgroup\$
0

2 Answers 2

6
\$\begingroup\$

DRY - don't repeat yourself - basically means that you should extract repeated logic into methods.

In this case you're repeating

  • checking validity
  • creating Date instances (if they're valid, you should just create them, and then compare them; it's madness to create them for the comparison only to re-create them a few lines later)
  • ... and for each of those, you're extracting 3 specific values from a params hash

So you could for instance do this instead:

# Returns a Date, or nil if the input isn't valid
def params_to_date(params = {})
 args = %w(year month day).map { |k| params[k] }
 Date.new(*args) if Date.valid_date?(*args)
end
# ...
def some_controller_action
 from_date = params_to_date(params[:from])
 to_date = params_to_date(params[:to])
 if from_date && to_date && from_date < to_date
 run_report(from_date, to_date)
 else
 redirect_to :back, alert: "Please provide valid date values."
 end
end
answered Nov 9, 2015 at 18:03
\$\endgroup\$
2
  • 1
    \$\begingroup\$ You can write Date.new(*args) rescue nil so validity isn't checked twice (by new and valid_date?) \$\endgroup\$ Commented Nov 12, 2015 at 21:00
  • \$\begingroup\$ @Spike Good point. I wasn't sure how/if the initializer handled invalid dates (e.g. if invalid dates are just coerced, like 2015-12-32 -> 2016年01月01日) and I didn't bother to check, so I just went with a safe solution of calling valid_date? first \$\endgroup\$ Commented Nov 12, 2015 at 23:58
0
\$\begingroup\$

As DRY is already mentioned, another one

def validate_date(date)
 formats = ['%d-%m-%y', '%Y-%m-%dT%H:%M:%S%z', '%Y-%m-%d %H:%M:%S %z']
 formats.each do |format|
 begin
 return true if Date.strptime(date, format)
 rescue
 end
 end
 raise InvalidInputError.new("Date you have entered is invalid, please enter a valid date")
end
answered Nov 15, 2015 at 10:34
\$\endgroup\$
1
  • \$\begingroup\$ Another what? Can you please explain the improvement? \$\endgroup\$ Commented Nov 15, 2015 at 10:58

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.