<p>My main concern would be to rewrite the subscribers table to be more idiomatic, namely to have a "diary_entry_subscriptions" table with user_id and diary_entry_id. Note the change from _subscribers to _subscriptions is subtle but reasonably important. You can create and delete a subscription, for example, and that makes more sense than to "create a subscriber".</p>

<p>This also allows the relationships to be easier to understand, e.g. a diary_entry <code>has_many :subscriptions, :through => :diary_entry_subscriptions</code> and <code>has_many :subscribers, :through => :subscriptions</code> or similar. A User <code>has_many :subscribed_diaries, :through => :diary_entry_subscriptions</code> etc. Then diary_entry.subscribers would still be a list of users, for example. Having to specify the :join_table and :association_foreign_key is a code smell, so best avoided, especially given we're creating a fresh table rather than dealing with a legacy structure.</p>

<p>The <a href="https://github.com/entry" class="user-mention">@entry</a>.subscribers << <a href="https://github.com/user" class="user-mention">@user</a> is also non-idiomatic. Something more like <a href="https://github.com/entry" class="user-mention">@entry</a>.subscriptions.create(user: <a href="https://github.com/user" class="user-mention">@user</a>) would be preferred.</p>

<p>Anything involving <code>user.id</code> in the controllers should be avoided - again, it's a minor code smell, and there's very likely to be an alternative approach that just uses the User objects.</p>

<p>If you want to go full-on clean-code, then the subscribe and unsubscribe POST targets should be handled by a DiaryEntrySubscriptionsController, which would have just <code>create</code> and <code>delete</code> methods (i.e. you are creating and deleting subscription objects) rather than adding even more non-standard methods to the DiaryEntryController. Let me know if you want to discuss this further.</p>

<p>I realise that many non-idiomatic approaches are used throughout the codebase, but I'd like to not let the situation get any worse!</p>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1309#issuecomment-251378359">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABWnLXTMAt3Xu5g1HXQurcUqUBUtUR-Bks5qwktugaJpZM4KNPYt">mute the thread</a>.<img alt="" height="1" src="https://github.com/notifications/beacon/ABWnLedkHg75V87A10G47egMbpS_jtV9ks5qwktugaJpZM4KNPYt.gif" width="1" /></p>
<div itemscope itemtype="http://schema.org/EmailMessage">
<div itemprop="action" itemscope itemtype="http://schema.org/ViewAction">
  <link itemprop="url" href="https://github.com/openstreetmap/openstreetmap-website/pull/1309#issuecomment-251378359"></link>
  <meta itemprop="name" content="View Pull Request"></meta>
</div>
<meta itemprop="description" content="View this Pull Request on GitHub"></meta>
</div>

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/openstreetmap/openstreetmap-website","title":"openstreetmap/openstreetmap-website","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/openstreetmap/openstreetmap-website"}},"updates":{"snippets":[{"icon":"PERSON","message":"@gravitystorm in #1309: My main concern would be to rewrite the subscribers table to be more idiomatic, namely to have a \"diary_entry_subscriptions\" table with user_id and diary_entry_id. Note the change from _subscribers to _subscriptions is subtle but reasonably important. You can create and delete a subscription, for example, and that makes more sense than to \"create a subscriber\".\r\n\r\nThis also allows the relationships to be easier to understand, e.g. a diary_entry `has_many :subscriptions, :through =\u003e :diary_entry_subscriptions` and `has_many :subscribers, :through =\u003e :subscriptions` or similar. A User `has_many :subscribed_diaries, :through =\u003e :diary_entry_subscriptions` etc. Then diary_entry.subscribers would still be a list of users, for example. Having to specify the :join_table and :association_foreign_key is a code smell, so best avoided, especially given we're creating a fresh table rather than dealing with a legacy structure.\r\n\r\nThe @entry.subscribers \u003c\u003c @user is also non-idiomatic. Something more like @entry.subscriptions.create(user: @user) would be preferred.\r\n\r\nAnything involving `user.id` in the controllers should be avoided - again, it's a minor code smell, and there's very likely to be an alternative approach that just uses the User objects.\r\n\r\nIf you want to go full-on clean-code, then the subscribe and unsubscribe POST targets should be handled by a DiaryEntrySubscriptionsController, which would have just `create` and `delete` methods (i.e. you are creating and deleting subscription objects) rather than adding even more non-standard methods to the DiaryEntryController. Let me know if you want to discuss this further.\r\n\r\nI realise that many non-idiomatic approaches are used throughout the codebase, but I'd like to not let the situation get any worse!\r\n\r\n\r\n\r\n"}],"action":{"name":"View Pull Request","url":"https://github.com/openstreetmap/openstreetmap-website/pull/1309#issuecomment-251378359"}}}</script>