DEV Community

Discussion on: The Rails Model Introduction I Wish I Had

Collapse
 
leastbad profile image
leastbad

Hi Max, I have some tips for you.

In your by_same_author example, you run reject against a collection of results. This is not very efficient because you're running a query and then enumerating through would could be a large collection. Instead, you could have Postgres do it all for you:

Book.where(author: author).where.not(id: id)

You also pluralized Book to Books, which I suspect isn't what you want.

Your add/remove ebook status example is technically correct - it runs and does what it says - but is not idiomatic Ruby in the sense that in the wild, we don't really create methods to update the function and mark them with bangs, which connotes "danger". If we did, we'd end up with extremely bloated model classes. Instead, it's far more common to just set attributes until you're ready to commit by calling save and handle any validation errors. By wrapping individual purpose-defined updates in methods, you're actually just creating a maintenance headache and making it harder to see validation errors or wrap these updates in a transaction.

Even if validations and transactions don't matter to you, consider that there's an even-shorter mechanism to do what you're doing:

update_attribute :ebook_version, true

There are lots of good reasons to use class methods in a model, but running a where operation is actually not one of them! I know that you followed up this example with a scoped version of the same, but I would argue that you shouldn't even suggest the class method version of this where, because it is so much less useful and more likely to cause bugs in the future.

Your same_genre scope is also taking a result from Postgres and running it through a Ruby enumerator when you could just get Postgres to run it all as a single query, and it will be much faster, cleaner and more composable in the future. In your example, you could do:

scope :same_genre -> (genres) {
  joins(:genres).where(genres: {id: genres.ids})
}

and instead of calling Ruby sample:

scope :random -> (num) {
  order("random()").limit(num)
}

Finally, you mentioned keeping validations in a separate class, and I just wanted to say that in practice this is not super common because if your validations are that sophisticated, you might have other refactoring to do, first.

At any rate, you might want to check out my Optimism gem for a powerful way to display validation errors in your forms via websockets.

Collapse
 
maxwell_dev profile image
Max Antonucci

Thank you feedback! I did some edits to the post that worked in the points you made.

Collapse
 
leastbad profile image
leastbad

Right on. :)

I guess my question is... a few of the things I suggested were all fixing the same fundamental concern, which is running a SQL query (efficient!) and then taking the result and iterating over it in Ruby (not efficient!). Do you feel confident that you've internalized how to properly kick ass moving forward?