1
\$\begingroup\$

I have this before_action method :

  def convert_duration_reminder
    allowed_duration_units = %(hour day)
    if allowed_duration_units.include? params[:store][:store_setting_attributes][:reminder_unit]
      store_settings_params = params[:store][:store_setting_attributes]
      @duration_reminder = store_settings_params[:reminder_duration].to_i.send(store_settings_params[:reminder_unit]).to_i / 3600
    else
      redirect_to store_settings_store_path(id: current_store.id)
    end
  end

I have a little bit of logic inside. The @duration_reminder is after used in an update_attributes.

Does it belongs in the controller or the model? I think it belongs in the controller because I'm using params and I have a redirect and methods are short (except line length...) and it doesn't process any data in database.

In the book Rails 4 action, it says:

  • Models contain the domain logic of your application. This logic dictates how the records in your database are retrieved, validated, or manipulated. In Rails appli- cations, models define the code that interacts with the database’s tables to retrieve and set information in them. Domain logic also includes things such as validations or particular actions to be performed on the data.

  • Controllers interact with the models to gather information to send to the view. They’re the layer between the user and the database. They call methods on the model classes, which can return single objects representing rows in the data- base or collections (arrays) of these objects. Controllers then make these objects available to the view through instance variables. Controllers are also used for permission checking, such as ensuring that only users who have special permission to perform certain actions can perform those actions, and users without that permission can’t.

\$\endgroup\$
  • 3
    \$\begingroup\$ My Ruby is rusty, but not rusty enough to not be able to read it; I have no idea what your code is doing. Could you please explain the purpose of it? At any rate, I don't think this is going to be a good fit for CodeReview. This question appears to be asking about a conceptual problem, rather than asking for a review in general (please correct me if I am wrong). \$\endgroup\$ – Dan Pantry Oct 26 '15 at 9:27
  • \$\begingroup\$ Thanks @DanPantry. Will mode the code somewhere else. This part of code simply convert time to minutes. The user can choose in the view between hours and days. Time in database will be in minutes. \$\endgroup\$ – Mio Oct 26 '15 at 9:40
1
\$\begingroup\$

It could go either way; controller or model. Or a service object, though that'd just be a cop-out.

However, note that a "model" is just structured data and associated methods - it doesn't have to be in a database. It's just the most common use of models in a Rails app. But really, the MVC pattern itself doesn't care about databases; a model's a model.

But I'd say there are other issues to tackle first. Like line length. Also that the method is doing several things:

  • It's validating params
  • It's converting input
  • It's assigning values
  • It's redirecting

That's way too much, if you ask me, though the first two kinda go together. It's also got a magic number (though 3600 is a fairly well-known one), and a magic array.

You're also saying, in a comment, that it "converts time to minutes". But dividing seconds by 3600 yields hours - not minutes. So there's something weird there too.

Anyway, I'd do something like:

DURATION_UNITS = %w(day hour).freeze
SECONDS_PER_HOUR = 60 * 60

def reminder_duration
  duration = params[:store][:store_setting_attributes][:reminder_duration]
  unit = params[:store][:store_setting_attributes][:reminder_unit]
  if DURATION_UNITS.include? unit
    duration.to_i.send(unit).to_i / SECONDS_PER_HOUR
  end
end

You'll either get a number or nil back. You can then assign or redirect as you see fit.

Still not doing great on the line length, though.

I'd consider adding a (non-ActiveRecord) model called Store or Settings or something, and instantiate that with the params hash. Then let the class handle things from there. Something like:

class Store
  DURATION_UNITS = %w(day hour).freeze
  SECONDS_PER_HOUR = 60 * 60

  def initialize(params_hash)
    ...
  end

  def reminder_duration
    ...
  end
end

Then your controller might look like:

before_action :load_settings

def some_action
  if @settings.reminder_duration
    # do stuff
  else
    redirect_to ...
  end
end

private

def load_settings
  @settings = Store.new(params[:store]) # or something
end

Point is you add a level of abstraction and indirection. Your controller is no longer dealing with a complex params hash; that's all hidden away.

That being said, you've still got a really complex params hash, so that Store class (or whatever you call it) still risks becoming a ball of mud. There's only such much you can do to keep things straight; I'd recommend trying to pare that down to something more manageable first.

\$\endgroup\$
  • \$\begingroup\$ Whoao. Thanks @Flambino. You were right it's hours and not minutes. I've edited the question. You deserve huge amount of bounty ! class Store already exist, so I need maybe to create something specific for my params. So for you the redirect belongs to the called method not in separate before_action method ? I often do that in before_action to avoid too much if in the called method in the controller. I will definitely follow the advices with naming, constant, etc. Thanks a lot. \$\endgroup\$ – Mio Oct 27 '15 at 9:34
  • \$\begingroup\$ The reminder duration absolutely, without question, needs to go in the model. This calculation has to do with the Domain of the application, and Domain Logic should go in the Models. \$\endgroup\$ – Greg Burghardt Oct 27 '15 at 13:37
  • \$\begingroup\$ @BeniMio You can redirect in the filter method, if you want - in itself that's perfectly fine to do. However, the way I structured things, you don't know if the reminder_duration is nil until you ask for it. Which you'd do in the action method. I chose to limit the filter method to 1 job: Set the instance variable. Everything else should then happen where it makes the most sense. And to me checking the value makes more sense in an action than in another filter method. \$\endgroup\$ – Flambino Oct 27 '15 at 15:45
  • \$\begingroup\$ @Flambino ok. Looks good for me. \$\endgroup\$ – Mio Oct 27 '15 at 15:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.