CODEWITHSUNDEEP
ruby-on-railsmodel-view-controller
atmaish
atmaish

Reputation: 2613

Best practices for coding in controller and models?

I have this short and simple code for sending an email notification to the user when someone comments on his post. What I'm concerned about is the location of this snippet.

if user.settings.enabled_notifications && some_other_conditions
    NotificationMailer.notify_topic_owner(comment,owner)
end

notify_topic_owner() just shoots a mail according to the parameters passed to it.

Basically, some_other_conditions contain some 3-4 conditions to be evaluated to true so as to send a mail. So clearly a controller isn't the right place for this code (I read somewhere that a controller code should be light and clean). I dont think i can move this snippet to a helper as helpers contain code for views. Again, models dont look right either as the code is not really about the model (or is it?).

Do I make a new module for this short snippet? Going forward, I would really appreciate if you could also tell about the best practices or some reference for such dull confusions. I find myself struggling with this quite often!

Upvotes: 0

Views: 184

Answers (3)

Zabba
Zabba

Reputation: 65467

You are asking the right questions. Why not go one step further and attempt to do some OOP : (the code below is not ideal, but it should give you a good idea of how to approach it). I have not taken "some_other_conditions" into consideration because those are likely something you know best where it will fit into your domain logic.

# A class for notification. I usually avoid depending directly on xxxMailer and similar
class Notifier

  # Inject the recipient
  def initialize(recipient)
    @recipient = recipient
  end

  def topic_commented(comment)
    # Only let Notifier know that NotificationMailer exists. (not perfect OOP. could inject this too)
    NotificationMailer.notify_topic_owner(comment,@recipient) if @recipient.notifications_enabled? # Ideally should be telling, not asking. Oh well.
  end


end



class User
  # Sprinkling of Law of Demeter
  def notifications_enabled?
    settings.enabled_notifications
  end
end

You call Notifier.new(current_user).topic_commented("Hello World"). In future, the topic_commented can send SMS, smoke signals, print, write to database etc. all without you having to change the calling code lke NotificationMailer.xxxx in many places.

Upvotes: 3

skinnynerd
skinnynerd

Reputation: 693

The convention I use to think about it is: "Should the mail be sent every time a comment is added, no matter by what action?". Think about whether if, in the future, you implemented an automated system that added comments, the mail should be sent in that case. If so, it's probably model code; otherwise, it's related to the way in which the comment was added, and it's controller code.

Upvotes: 1

Ultimation
Ultimation

Reputation: 1069

I don't see what would be wrong with putting this in a controller. If it's related to a method in your controller, it can definitely go there. If it's called after a save or something, you can probably move it to the model.

Generally I think the best practice is try to put as much stuff into models and classes as possible. Save the controller for controller specific code, and helpers should only contain code related to rendering content in views. A lot of times, I'll take code in my controller and move it to the model while refactoring. My opinion anyway :)

Upvotes: 1

Related Questions