CODEWITHSUNDEEP
ruby-on-railsrubyruby-on-rails-4activerecord
Oliver
Oliver

Reputation: 1181

Rails: Global instance variables in Rails

In my app users can submit recipes through a form, which will be published on a website. Before recipes get published they are moderated through a moderator.

Therefore my app shows in the navbar a count of all currently unpublished recipes for the moderator like so:

enter image description here

To achieve this at the moment I do the following:

application.rb

before_action :count_unpublished

def count_unpublished
  @unpublished_count = Recipe.where(:published => false).count
end

_navbar.html.erb

<li>
  <%= link_to recipes_path do %>
     Recipes <span class="badge" style="background-color:#ff7373"><%= @unpublished_count %></span>
  <% end %>
</li>

It works, but I am wondering now if this is a good practice as now with every action my app hits the recipe database (which is maybe not very elegant).

Is there a better solution to achieve this?

Upvotes: 2

Views: 405

Answers (3)

Alexis
Alexis

Reputation: 364

Your falling into the trap of premature optimisation. Before doing any optimisation (which increases code complexity most of the time) you have to profile your code to find the bottleneck. Improving a SQL requests which counts for a small part of the total response time is useless. In the contrary if the SQL takes a big amount of time, that is a great improvement.

For that I can recommend these 2 gems:

To reply to your question, the better way would be:

# app/models/recipe.rb
class Recipe < AR::base
  # A nice scope that you can reuse anywhere
  scope :unpublished, -> { where(published: false) }
end

Then in your navbar.html.erb:

<li>
  <%= link_to recipes_path do %>
     Recipes <span class="badge" style="background-color:#ff7373"><%= Recipe.unpublished.count %></span>
  <% end %>
</li>

You have no more these ugly callback and instance variable in the controller.

Unless you have a lot of recipes (something like 100K or more) performance won't be an issue. In that case you can add an index:

CREATE INDEX index_recipes_unpblished ON recipes(published) WHERE published='f'

Note that the index applies only when published is false. Otherwise it would be counter productive.

I think that caching in your case is not well because the invalidation is extremely complex and leads to awful and easy breakable code. Don't worry to hit the database, we will never write faster code than PostgreSQL/MySQL, etc.

Upvotes: 0

Abdullah
Abdullah

Reputation: 2111

cache_key = "#{current_user.id}_#{unpublished_count}"
@unpublished_count = Rails.cache.fetch(cache_key, expires_in: 12.hours) do 
  Recipe.where(:published => false).count
end

For More: http://guides.rubyonrails.org/caching_with_rails.html#low-level-caching

Upvotes: 4

Sergio Tulentsev
Sergio Tulentsev

Reputation: 230356

To avoid hitting the database, you can introduce caching. It comes in many forms: faster storage (memcached, redis), in-process caching (global/class variables) and so on. And they all share the same problem: you need to know when to invalidate the cache.

Take a look at this guide to get some ideas: Caching with Rails.

If I were you, I would not care about this until my profiler tells me it's a performance problem. Instead, I'd direct my efforts to developing the rest of functionality.

Upvotes: 3

Related Questions