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
- make sure the dates themselves are valid, i.e., no blank inputs, and nothing like February 30th, then
- 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)
2 Answers 2
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
-
1\$\begingroup\$ You can write
Date.new(*args) rescue nil
so validity isn't checked twice (bynew
andvalid_date?
) \$\endgroup\$Spike– Spike2015年11月12日 21:00:16 +00:00Commented 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\$Flambino– Flambino2015年11月12日 23:58:54 +00:00Commented Nov 12, 2015 at 23:58
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
-
\$\begingroup\$ Another what? Can you please explain the improvement? \$\endgroup\$janos– janos2015年11月15日 10:58:50 +00:00Commented Nov 15, 2015 at 10:58
Explore related questions
See similar questions with these tags.