Skip to content

Instantly share code, notes, and snippets.

@bhserna
Last active December 10, 2015 14:38
Show Gist options
  • Star 1 You must be signed in to star a gist
  • Fork 1 You must be signed in to fork a gist
  • Save bhserna/4448661 to your computer and use it in GitHub Desktop.
Save bhserna/4448661 to your computer and use it in GitHub Desktop.
This are different examples to do the same thing, "mark a notification as read", and the pros and cons that I see in each example

Where is the app?

This are different examples of doing the same thing, "mark a notification as read", and the pros and cons that I see in each example

1. - Logic in the model

  • The user clicks the notification link
  • The system POSTs to the action "notifications/:id/mark_as_read"
  • The controller calls the method Notification#mark_as_read
  • The model updates the attribute "read" saves the Notification record
  • And sends the response back to the client
class App.NotificationView

  constructor: (@el, @notification) ->
    @el.click => @notification.markAsRead()
      
class App.Notification extends App.Model

  markAsRead: ->
    unless @get('read')
      $.ajax
        url: "notifications/#{@get 'id'}/mark_as_read"
        type: 'POST'
        dataType: "json"
        success: (json) =>
          @set('read', json.read)
class NotificationsController < ApplicationController
  def mark_as_read
    @notification = Notification.find(params[:id])
    @notification.mark_as_read
    return json: @notification
  end
end

class Notification < ActiveRecord::Base
  def mark_as_read
    update_attributes read: true
  end
end

Pros

  • Logic in one place
  • Can be part of a domain model in the server

Cons

  • May be slow, because we have to wait to the server

2.- Logic in the controller

  • The user clicks the notification link
  • The client POSTs to the action "notifications/:id/mark_as_read"
  • The controller updates the attribute "read" of the notification record and then saves it
  • And sends the response back to the client
class App.NotificationView

  constructor: (@el, @notification) ->
    @el.click => @notification.markAsRead()
      
class App.Notification extends App.Model

  markAsRead: ->
    unless @get('read')
      $.ajax
        url: "notifications/#{@get 'id'}/mark_as_read"
        type: 'POST'
        dataType: "json"
        success: (json) =>
          @set('read', json.read)
class NotificationsController < ApplicationController
  def mark_as_read
    @notification = Notification.find(params[:id])
    @notification.update_attributes read: true
    return json: @notification
  end
end

Pros

  • Logic that is only used in that case does not go in the model
  • Looks like the controller serves only to call a method from the client

Cons

  • May be slow because we have to wait to the server

3.- Logic in the client

  • The user clicks the notification link
  • The Notification#mark_as_read, method of a client model, that modifies the "read" attribute (backbone.js or something like that)
  • The client updates to "notifications/:id" in the server
class App.NotificationView

  constructor: (@el, @notification) ->
    @el.click =>
      @notification.markAsRead()
      @notification.save()
      
class App.Notification extends App.Model

  markAsRead: ->
    @set('read', true)
class NotificaitiosController < ApplicationController
  def update
    @notification = Notification.find(params[:id])
    @notification.update_attributes(params[:notificaiton])
  end
end

Pros

  • Logic in one place
  • Can be part of a domain model in the client
  • Fast, because we don`t wait to the server

Cons

  • Not sure about business logic, in the client

4.- Logic client and server

  • The user clicks the notification link
  • The Notification#mark_as_read, method of a client model, that modifies the "read" attribute (backbone.js or something like that)
  • The client POSTs to the action "notifications/:id/mark_as_read"
  • The controller calls the method Notification#mark_as_read
  • The model updates the attribute "read" and saves the Notification record
  • And sends the response back to the client
class App.NotificationView

  constructor: (@el, @notification) ->
    @el.click => @notification.markAsRead()
      
class App.Notification extends App.Model

  markAsRead: ->
    @set('read', true)
    $.ajax
      url: "notifications/#{@get 'id'}/mark_as_read"
      type: 'POST'
      dataType: "json"
      success: (json) =>
        @set('read', json.read)
class NotificationsController < ApplicationController
  def mark_as_read
    @notification = Notification.find(params[:id])
    @notification.mark_as_read
    return json: @notification
  end
end

class Notification < ActiveRecord::Base
  def mark_as_read
    update_attributes read: true
  end
end

Pros

  • Fast, because we don`t wait to the server

Cons

  • Logic in many places
  • Maybe a nightmare is comming
@harlow
Copy link

harlow commented Jan 4, 2013

Personally I'd go for #4 with a few small adjustments:

For the controller, consider doing a nested route. You would access the route by posting to /notifications/1/acknowledgements and then access the notification id with params[:notification_id].

Note: If there are multiple users in the system you may want to find the notification based on the current_user as opposed to accessing directly through the Notification model. i.e. current_user.notifications.find(params[:notification_id])

I was having trouble coming up with a name for the controller. I landed on acknowledgements thinking that the Notification has been acknowledged once it has been read. There may be a better name for it.

# app/controllers/acknowledgements_controller.rb
class AcknowledgementsController < ApplicationController
  def create
    @notification = Notification.find(params[:notification_id])
    @notification.mark_as_read
    return json: @notification
  end
end

If you ever decide to have an option to check as unread then you could just add a DELETE action to this controller and mark_as_unread.

You always want to have data validation on your server side.

@e3matheus
Copy link

I like number 3 the most.

Even do the business logic is in the client, its still in a "model". If the notification model has no validations, I don´t see a problem with directly calling it from the client side.

I don´t like number 2, because the controller knows too much about how the object is persisted. In my opinion, it should only know what to do, not how to do it.

What I don't like about Number 1 and Number 4 is that the implementation details are in two places. You are binding to the fact that the implementation is a boolean called "readed"(a name change to "read" or another type of persistence would cause several places in your codebase to change).

@bhserna
Copy link
Author

bhserna commented Jan 4, 2013

@harlow yes I made a mistake, the route should be "notifications/1/mark_as_read" (I'm gonna update ir =) ), but I think that if is just one action that is not "REST", I would keep it in the same controller...

But I do think that is better to do

current_user.notifications.find(params[:id])

Why do you prefer the number #4? ... Don't you feel that is a little funky to have the "mark_as read" logic in two places?

@e3matheus I really like the third also, but I think is good in this case, but I don't know in other cases with more logic.

What do you think about the number #1 if we do something like this

 markAsRead: ->
    unless @get('read')
      $.ajax
        url: "notifications/#{@get 'id'}/mark_as_read"
        type: 'POST'
        dataType: "json"
        success: (json) =>
          this = new App.Notification(json)

Just build an other notification with the server response?

@e3matheus
Copy link

I agree with you. If you need more logic(like searching inside the current users notifications), I would go with creating a controller action that marks the notification as read with the change you made(Really liked the change btw)

@eddie-ruva
Copy link

I would go for either the first or the third option. What I like about the third option is that is going to be fast because you are going to keep all your logic on the client side and the work that you are doing is not too heavy but I think that if your whole application is keeping the logic on the backend you should keep doing it that way so you don't end with logic with both places. I also like the first option because it's very straightforward and easy to maintain. I don't think the slowness in the response from the server is a setback because for me as a user to see if my notification was marked as read is not that big of a deal.

@bhserna
Copy link
Author

bhserna commented Jan 4, 2013

@eddie-ruva I think that is the hard stuff, when you have almost all your logic in the server, but you think that the client side implementation is simpler.

@adriancuadros
Copy link

Hey,

Great post since I don't think we take into account all the possibilities of accomplishing small functionality like this all the time.

In most applications I've dealt with, I would go with number 3 no questions asked. Generally the update method works for all publicly accessible and settable attributes. If the "read" attribute is publicly settable with no prior weird or special conditions I think this would be the way to go.

If for some reason you want to do something special extra, like sending an e-mail to notifier or something like that, maybe I would go with number 1 adding the special context behaviour.

Thanks for sharing

@bhserna
Copy link
Author

bhserna commented Jan 4, 2013

@adriancuadros I like your point of view, about the case when you need to send an email or make an other process ... Because maybe if you go with the option 3, you will have to implement a kind of listener, to know what changes, are happening in each situation (something like this http://devblog.avdi.org/2012/11/12/rubytapas-episode-21-domain-model-events/ .... well maybe not that complex =) ).

thanks for help me to change "readed" for "read" =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment