HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyrailsMinor

Checking for valid date range in Rails

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
rangecheckingdaterailsforvalid

Problem

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)

Solution

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

Code Snippets

# 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

Context

StackExchange Code Review Q#110262, answer score: 6

Revisions (0)

No revisions yet.