Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added some model issues #4

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Conversation

Zawgor24
Copy link
Contributor

@Zawgor24 Zawgor24 commented May 6, 2019

No description provided.

@Zawgor24 Zawgor24 requested a review from stgeneral May 6, 2019 09:13
@Zawgor24 Zawgor24 self-assigned this May 6, 2019
@@ -7,6 +7,9 @@ The following is a base template for an ActiveRecord model `Customer`.
* Avoid ActiveRecord callbacks where possible, prefer Service Objects architecture to store any business
* Order associations, scopes and validations alphabetically
* Aim to remove business logic that is not directly related to reading from or writing to the database
* `Scopes` vs `Class methods`. Use scopes when the logic is very small, for simple where/order clauses,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have Query Objects for complex queries, sqls, and "scopes" - https://github.com/ergoserv/handbook/blob/master/guides/query_objects.md.
I'd suggest to use them instead of Class methods. What do you think?

@@ -77,4 +80,20 @@ class Customer < ActiveRecord::Base
# some calculation
end
end

# app/models/concerns/commentable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems the file extenstion is missing.


# app/models/concerns/commentable
module Commentable
# Concern should be named with `able` prefix
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest moving this message to the line about the concerns above (in Key Points section) or add a Conventions (like in some other articles - https://github.com/ergoserv/handbook/blob/master/guides/query_objects.md#conventions).

module Commentable
# Concern should be named with `able` prefix
extend ActiveSupport::Concern
# EXTENDing is required
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably we can omit this comment because this is known from the official Rails guides. You may add a link to concerns guide page somewhere instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants