Checking parameters

Hi.

I am often checking the same parameters (like param[:page] and param[:per_page]) in models over and over again (are those in a specific range ... if not use defaults): page = !options[:page].blank? && options[:page] =~ /^[0-9]+$/ && options[:page].to_i > 0 ? options[:page].to_i : 1 per_page = !options[:per_page].blank? && options[:per_page] =~ /^[0-9]+ $/ && options[:per_page].to_i > 0 && options[:per_page].to_i <= 100 ? options[:per_page].to_i : 10

How can I make this more DRY and easier to read?

turkan wrote in post #964861:

Hi.

I am often checking the same parameters (like param[:page] and param[:per_page]) in models over and over again (are those in a specific range ... if not use defaults): page = !options[:page].blank? && options[:page] =~ /^[0-9]+$/ && options[:page].to_i > 0 ? options[:page].to_i : 1 per_page = !options[:per_page].blank? && options[:per_page] =~ /^[0-9]+ $/ && options[:per_page].to_i > 0 && options[:per_page].to_i <= 100 ? options[:per_page].to_i : 10

How can I make this more DRY and easier to read?

Well, first of all, the code you provided is a classic example of obfuscated Ruby. You can make it more readable by using if instead of ?: .

Now, as far as the repetition of the conditions themselves, just do the same thing you'd do for any other repeated code: extract it into a method of its own.

Best,

You can make it easier to read by using simpler constructs such as if. It will take up a few more lines of source code but will not have any measurable effect on execution time, and will be much easier for those that come after you to understand.

You can DRY it by putting it in a method.

I also have to wonder why a _model_ is worrying about page number and per_page.

Colin

You can DRY it by putting it in a method.

In what method should I put it that is visible in all models? In an extra lib class?

I also have to wonder why a _model_ is worrying about page number and per_page.

I pass it to a model method that does some Sphinx search. What would be a better way? The search expression is so long, so I thought it would be good to keep it out of my controller (everybody says "keep your controller thin").

turkan wrote in post #964900:

You can DRY it by putting it in a method.

In what method should I put it that is visible in all models? In an extra lib class?

Probably.

I also have to wonder why a _model_ is worrying about page number and per_page.

I pass it to a model method that does some Sphinx search. What would be a better way? The search expression is so long, so I thought it would be good to keep it out of my controller (everybody says "keep your controller thin").

Then you probably want a separate model to talk to Sphinx, then call Sphinx.search(params[:criteria]) in your controller. Or maybe you want a utility class to do this. It really depends on the structure of your app.

Best,

Thanks for the answers.

One last general question. Where do you validate your parameters fetched by "params" that should be passed to models? Do you check them right away in the controller and give them cleaned up to the model, or do you just provide them as they are (maybe just put them in single variables) and let the model check them?

Please quote when replying.

Kai Schlamp wrote in post #965115:

Thanks for the answers.

One last general question. Where do you validate your parameters fetched by "params" that should be passed to models? Do you check them right away in the controller and give them cleaned up to the model, or do you just provide them as they are (maybe just put them in single variables) and let the model check them?

It's usually the model's job to validate. That's what the validates_* methods are for.

Validation in the controller will lead to repetitive code scattered all over the application.

Best,

For a start, make use of the fact that Ruby is not Java. For instance:

''.to_i # => 0 nil.to_i # => 0 'blargh'.to_i # => 0 '123foo'.to_i # => 123

That last one is arguably weird, but reasonable.

Secondly, for range-ish cases you might want to use some of the Array functions. Your first case becomes:

[1, options[:page].to_i].max

the second (this is not as clear):

[100, [1, options[:per_page].to_i].max].min

or, if you don't mind scribbling on the options hash:

options[:per_page] = 10 unless (1..100).include? (options[:per_page].to_i)

--Matt Jones

Validation in the controller will lead to repetitive code scattered all over the application.

Ok, then one last last question :wink: ... just want to make sure that I do it the Rails way.

When you let the user pass several paramters regarding how the instances of your model are presented (order, page, and so on), where do you check those? Do make such (sometimes more complicated) queries in the controller (and check those params there) or define a special search method in the model (and check also those parameters in the model)?

What are you checking these parameters for, exactly? Everything has to pass through the model to the database, the model is responsible for sanitizing any SQL that is needed. Further, the model can enforce any business logic requirements you define in your validates* filters.

If you have a search facility in your page, you probably want to run that through either a separate search method in your controller, again, passing back whatever the user has entered to the model for eventual processing; or maybe you want a separate Search model and controller to interface with Sphinx or Solr or whatever you might be using under the hood. A separate model also allows you to search across multiple models without having to repeat a search controller method in each of those models' controllers.

Walter