Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm working on the Rails project where I have many-to-many :through relationship. Each setting has many notifications through email_notification model where the notification can be primary or a secondary.


create_table "email_notifications", :force => true do |t|
  t.integer "setting_id"
  t.integer "notification_id"
  t.boolean "primary"
  t.boolean "secondary"

create_table "notifications", :force => true do |t|
  t.string   "name"

create_table "settings", :force => true do |t|
  t.string   "truck_identification"
  t.string   "email_periods"
  t.integer  "email_recepient_id"
  t.datetime "created_at",                             :null => false
  t.datetime "updated_at",                             :null => false
  t.integer  "truck_fleet_id"

In models

class EmailNotification < ActiveRecord::Base
  attr_accessible :notification_id, :setting_id, :primary, :secondary
  belongs_to :notification
  belongs_to :setting

class Notification < ActiveRecord::Base
  attr_accessible :name

  has_many :settings, :through => :email_notifications
  has_many :email_notifications

class Setting < ActiveRecord::Base
  # TODO: refactor this bullshit
  attr_accessible :email_periods, :email_recepient_id, :truck_identification, 

  has_many :notifications, :through => :email_notifications
  has_many :email_notifications
  validates_uniqueness_of :truck_fleet_id 
  # validates :email, :email_format => true

I used this many-to-many relationship because I need flexibility to add up new Notifications over the time, and keep ability to make notifications flexible as possible. For example, I might add a column frequency, or interval etc.

Let me know if you can find a better solution.

share|improve this question

1 Answer 1

# TODO: refactor this bullshit

While amusing to devs, swear words probably don't belong in production code.

In your copypasta of schema.rb, I do believe you forgot to paste the end keywords.

As a side note, you can also write null: false instead of :null => false; as far as I can tell, this is the accepted syntax. You can do the same thing with through: :email_notifications and anything of the form Symbol => BasicObject. This syntax was added after this question was posted, but if you're still interested in updating it, that's what I'd do.

Personally, I prefer to use Symbols instead of Strings as table names, though that's entirely personal preference.

In your models, you also forgot an end at the end of your first class.

Aside from that... Well, honestly, it looks good to me. Since I can't really tell what Setting and Notification are supposed to represent on a more concrete level than their names alone can give, I can't give good advice on how to link them together. However, I don't see why you need a many-to-many, if I'm understanding the purpose of Setting and Notification correctly. It seems like it could be a one-to-many relationship, though I'm not sure which side should be the one.

share|improve this answer
To be fair to OP, schema.rb is auto-generated. It's not intended to be edited manually (the spacing is Rails' fault). Also, since it's a 3 year old question, the hashrocket syntax (=>) was perhaps the only syntax available at the time. The JSON-like key: value syntax didn't appear until Ruby 1.9. –  Flambino Jun 28 at 13:21
@Flambino Oh, I wasn't aware that schema.rb was autogenerated. I'm just gonna delete that point and pretend it never existed... (And yes, I'm aware of the latter. I had a sentence in there like "It was introduced in 1.9" somewhere but I guess it got lost while I was shuffling around the points in my answer.) Fixed both points. –  QPaysTaxes Jun 28 at 13:25

Your Answer


By posting your answer, you agree to the privacy policy and terms of service.

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