While reading through a Rails app, I came across the following chunk of Ruby code.
page_index = 1 page_index = params[:page].to_i if params[:page] options["page"] = page_index
It’s a piece of a larger controller method that handles pagination for an article. It checks the web request’s parameters for a “page” field, using that value if it exists; otherwise it defaults to the first page.
What’s with the to_i? Most likely the original author was worried about malicious users passing in garbage values since to_i will convert non-numeric values to 0. But how does the code handle -1? How does it handle 102 on a 5 page article? There’s a bigger validation problem here.
I reworked the code and replaced it with the following.
page_range = (1..6) # article with six pages options["page"] = (page_range === params[:page] && params[:page]) || 1
If the value of params[:page] is within the bounds of the page_range, set options["page"] to params[:page]; otherwise set it to 1. The === operator correctly handles non-numeric and nil values. This code performs the correct validation in fewer lines.
As a larger refactoring I replaced the options hash with a presenter, renamed the presenter’s variables to make them more readable, and encapsulated the page check code into a method.
presenter.page_number = find_page_number
Of course, by adding the find_page_number method I’ve increased the number of lines of code, but it makes the controller’s code more self-documenting so I prefer it.