Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Model method names should not conflict with attribute names #74

Merged
merged 1 commit into from Apr 19, 2013

Conversation

Projects
None yet
3 participants
Owner

mike-burns commented Mar 15, 2013

No description provided.

@croaky croaky commented on an outdated diff Mar 15, 2013

best-practices/README.md
@@ -56,6 +56,7 @@ Rails
change can be solved with another migration.
* Validate the associated `belongs_to` object (`user`), not the database
column (`user_id`).
+* Don't name methods after database columns in the same class.
@croaky

croaky Mar 15, 2013

Owner

The guidelines are alphabetized within each section.

Owner

croaky commented Mar 15, 2013

Might be nice to add a "why" in the commit message.

Owner

mike-burns commented Mar 18, 2013

Should we have "why"s listed in the document itself, so people can more easily know what to watch for when they violate a best practice?

Owner

jferris commented Mar 18, 2013

I'd be concerned that we can succinctly describe the reasons for some of these guidelines, which would make the guide longer and harder to scan. Having the reasoning in the commit message means that you can use git blame to discover the original reason without having it in the actual guide body. Thoughts?

Owner

mike-burns commented Mar 18, 2013

That works. I'll push a commit with a longer explanation later.

Owner

mike-burns commented Mar 22, 2013

Squashed and gave an explanation.

@jferris jferris commented on an outdated diff Mar 22, 2013

best-practices/README.md
@@ -54,6 +54,7 @@ Rails
`update_attribute`, and `toggle`.
* Don't change a migration after it has been merged into master if the desired
change can be solved with another migration.
+* Don't name methods after database columns in the same class.
@jferris

jferris Mar 22, 2013

Owner

Can we rephrase this as an "Avoid" rule? I feel like that appropriately expresses the level of resistance developers should feel here.

Owner

jferris commented Mar 28, 2013

Good to merge.

Owner

jferris commented Apr 19, 2013

Was this ever merged?

@mike-burns mike-burns Model names distinct from database fields
Naming a method in an ActiveRecord object after the DB column causes
issues with Rails validations such as presence and uniqueness. Even if
the object does not have those validations now, wrapping an AR attribute
limits people in the future arbitrarily.

Instead of wrapping a column, instead name the column more simply. For
example, if you need to expose the receipt string as an `AppleReceipt`,
store the value as `encoded_receipt` or `receipt_string`, then provide a
`receipt` method that handles the wrapping.
eea029c

@mike-burns mike-burns merged commit eea029c into master Apr 19, 2013

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