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

Use @collection instead of @collection.present? in some admin controllers #2046

Merged

Conversation

jordan-brough
Copy link
Contributor

I'm pretty sure defined? was the intended behavior.

Using present? can generate an extra query for the presence check, and will
also prevent the instance-variable caching from working when the query returns
no results.

I'm pretty sure `defined?` was the intended behavior.

Using `present?` can generate an extra query for the presence check, and will
also prevent the instance-variable caching from working when the query returns
no results.
Copy link
Contributor

@jhawthorn jhawthorn left a comment

Choose a reason for hiding this comment

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

👍 return @collection if @collection would also work (only because we know collection can't be nil or false)

This is a good example of blind use of present? being bad.

Should work equally well in these cases
@jordan-brough jordan-brough changed the title Use defined? instead of present? in some admin controllers Use @collection instead of @collection.present? in some admin controllers Jun 27, 2017
@jordan-brough
Copy link
Contributor Author

return @collection if @collection would also work

Good point, I've updated to that.

@gmacdougall gmacdougall merged commit 066a6da into solidusio:master Jun 28, 2017
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.

5 participants