Code smell – Double dipping Ruby on Rails controller actions
Something that I’ve encountered on some Rails projects that I have inherited, or done work on is single controller actions that serve multiple request types – for example, a /contact route that handles both initially displaying a contact form, via GET request and then a subsequent form post, via POST request.
I feel this is code smell. Intuitively, it feels like a something brought over from PHP or perhaps other pre-Rails web development appraoches; I seem to remember, that when I was learning PHP back in the day, I may learned that approach myself from PHP books. There are many potential problems with this approach, though, in my estimation.
Fat Model, Skinny Controller
Fat Model, Skinny Controller – this concept and the logic that follows from it, encourages organized code, by refactoring bloated controller actions into models, services, concerns, etc. When you have a single action handling your GET and POST, you’ve already probably doubled the size of it. And for what benefit?
It’s not RESTful
It’s not RESTful – if you’ve got these double-dipping actions in place, you may be missing an oportunity to work within the Rails way, and the RESTful approach. It may be a good idea to look at the action and the resource that it represents, and find out why you aren’t handling this with a resources :foo. If you have this sort of action in place because you’re working with a non-Active Record situation, take a look at moving to ActiveModel, which can function nicely as a resource.
Subject to change with updates
If you don’t have adequate test coverage this will bite you in the ass, if you do, you’ll still need to update your code and tests when updating Rails. Rails updates have meant that you can do longer simply say match ‘/contact’ => ‘contact#index’ and then have that route accept GET and POST requests. You’re going to have to update every route that uses this approach to something like match ‘/contact’ => ‘contact#index’, via: [:get, :post], Rails 4.0 also made changes such that PUT requests became PATCH requests. If your code is checking request.put? you will need to update your tests and controller logic to account for it. And god help you if you don’t have adequate test coverage in place.
I’m sure there are other problems with this approach that I can’t think of off the top of my head, or others still that I’m not even aware of. But one thing I know for sure, this has a distinct aroma of code smell in my opinion, and I encourage you to either split these double-dippers into discrete actions, or to rethink what you are representing and whether it can become a resouce.
I am available for Ruby on Rails consulting work – get in touch to learn more.