2
\$\begingroup\$

I have a Rails 3.2.14 app where I have a home controller (dashboard). In this controller and view I'm calling multiple instance variables to get different counts based off of scopes I've created in the Call and Unit model. I'd like to see if anyone has any suggestions on how I can DRY this up and query less so the controller/view loads faster. This code is very old and I'm looking for the best way to refactor it.

home_controller.rb

  def index
    @calls = Call.open_status
    @all = Call.all
    @unit = Unit.active.order("unit_name")
    @avail = Unit.active.in_service
    @unavail = Unit.active.out_of_service
    @unassigned = Call.unassigned_calls
    @today = Call.today
    @year = Call.year
    @previous = Call.previous_year
    @assigned = Call.assigned_calls.until_end_of_day
    @unassigned = Call.unassigned_calls.until_end_of_day
    @scheduled = Call.scheduled_calls
  end
end

call.rb model

 scope :open_status, where(call_status: "open")
  scope :cancel, where(call_status: "cancel")
  scope :closed, where(call_status: "close")
  scope :waitreturn, where(wait_return: "yes")
  scope :wc, lambda { where(service_level_id: ServiceLevel.find_by_level_of_service("WC").id) }
  scope :bls, lambda { where(service_level_id: ServiceLevel.find_by_level_of_service("BLS").id) }
  scope :als, lambda { where(service_level_id: ServiceLevel.find_by_level_of_service("ALS").id) }
  scope :micu, lambda { where(service_level_id: ServiceLevel.find_by_level_of_service("MICU").id) }
  scope :cct, lambda { where(service_level_id: ServiceLevel.find_by_level_of_service("CCT").id) }
  scope :assist, lambda { where(service_level_id: ServiceLevel.find_by_level_of_service("ASSIST").id) }
  scope :em, lambda { where(service_level_id: ServiceLevel.find_by_level_of_service("EM").id) }
  scope :by_service_level, lambda { |service_level| where(service_level_id: ServiceLevel.find_by_level_of_service(service_level).id) }
  scope :by_region, lambda { |region| where(region_id: Region.find_by_area(region).id) }
  scope :from_facility, lambda { |id| where(transfer_from_id: id) }
  scope :to_facility, lambda { |id| where(transfer_to_id: id) }
  scope :search_between, lambda { |start_date, end_date| where("transfer_date BETWEEN ? AND ?", start_date.beginning_of_day, end_date.end_of_day)}
  scope :search_by_start_date,  lambda { |start_date| where('transfer_date BETWEEN ? AND ?', start_date.beginning_of_day, start_date.end_of_day) }
  scope :search_by_end_date, lambda { |end_date| where('transfer_date BETWEEN ? AND ?', end_date.beginning_of_day, end_date.end_of_day) }
  scope :open_calls, lambda { open_status.includes(:call_units).where(["call_units.unit_id IS NOT NULL"]) }
  scope :unassigned_calls, lambda { open_status.includes(:call_units).where(["call_units.unit_id IS NULL"]).order("transfer_date ASC") }
  scope :assigned_calls, lambda { open_status.includes(:call_units).where(["call_units.unit_id IS NOT NULL"]).order("transfer_date ASC") }
  scope :by_unit_name, lambda {|unit_name| joins(:units).where('units.unit_name = ?', unit_name)}
  scope :ambulance, lambda {joins(:units).where('units.vehicle_type = ?', "Ambulance")}
  scope :wheelchair, lambda {joins(:units).where('units.vehicle_type = ?', "Wheelchair")}
  scope :scheduled_calls, lambda { open_status.includes(:call_units).where(["calls.transfer_date > ?", Time.zone.now.end_of_day]).order("transfer_date ASC") }
  scope :medic_calls, lambda { where(["call_status = ? and call_units.unit_id IS NOT NULL", "open"]).order("id ASC") }
  scope :today, lambda { where("transfer_date BETWEEN ? AND ?", Time.zone.now.beginning_of_day, Time.zone.now.end_of_day) }
  scope :yesterday, lambda { where("transfer_date BETWEEN ? AND ?", 1.day.ago.beginning_of_day, 1.day.ago.end_of_day) }
  scope :year, lambda { where("transfer_date BETWEEN ? AND ?", Time.zone.now.beginning_of_year, Time.zone.now.end_of_year) }
  scope :previous_year, lambda {where("transfer_date BETWEEN ? AND ?", 1.year.ago.beginning_of_year, 1.year.ago.end_of_year)}
  scope :until_end_of_day, lambda { where("transfer_date < ?", Time.zone.now.end_of_day) }

unit.rb model

  scope :in_service, lambda { where(status_id: Status.where(unit_status: ["In Service", "At Post", "At Station"]).map(&:id))}
  scope :out_of_service, lambda { where(status_id: Status.find_by_unit_status("Out of Service").id)}
  scope :active, where(unit_status: "Active")

home/index.html.erb

<div class="main-area dashboard">
    <div class="container">
        <div class="row">
            <div class="span12">
                <div class="slate clearfix">
                    <a class="stat-column" href="#">
                    <span class="number"><%= @today.count %></span>
                    <span>Today's Calls</span>

                    <span class="number"><%= @year.count %></span>
                    <span>Current YTD Calls</span>

                    <span class="number"><%= @previous.count %></span>
                    <span>Previous YTD Calls</span>


                    <span class="number"><%= @all.count %></span>
                    <span>Calls To Date</span>
                    </a>

                    <a class="stat-column" href="#">

                    <span class="number"><%= @calls.count %></span>
                    <span>Open Calls</span>

                    <span class="number"><%= @assigned.count %></span>
                    <span>Active Calls</span>

                    <span class="number"><%= @scheduled.count %></span>
                    <span>Scheduled Calls</span>

                    <span class="number"><%= @unassigned.count %></span>
                    <span>Unassigned Calls</span>
                    </a>

                    <a class="stat-column" href="#">

                    <span class="number"><%= @today.ambulance.count %></span>
                    <span>Ambulance Calls</span>

                    <span class="number"><%= @today.wheelchair.count %></span>
                    <span>Wheelchair Calls</span>

                    <span class="number"><%= @avail.count %></span>
                    <span>Units In Service</span>

                    <span class="number"><%= @unavail.count %></span>
                    <span>Units Out of Service</span>
                    </a>

                    <a class="stat-column" href="#">


                    <span class="number"><%= @today.bls.count %></span>
                    <span>BLS Calls</span>

                    <span class="number"><%= @today.als.count %></span>
                    <span>ALS Calls</span>

                    <span class="number"><%= @today.cct.count %></span>
                    <span>CCT Calls</span>

                    <span class="number"><%= @today.micu.count %></span>
                    <span>MICU Calls</span>
                    </a>
                </div>
            </div>
        </div>

        <div class="row">
            <div class="span6">
                <div class="slate">
                    <div class="page-header">
                        <h2><i class="icon-signal pull-right"></i>Medics</h2>
                        </div>
                            <table class="table table-striped table-bordered">
                                <thead>
                                    <tr>
                                    <th>Unit</th>
                                    <th>Attendant</th>
                                    <th>InCharge</th>
                                    </tr>
                                </thead>
                            <tbody>
                              <tr>
                                <% @unit.each do |unit| %>
                                <td><%= unit.try(:unit_name) %></td>
                                <td><%= unit.attendant.try(:medic_name) %></td>
                                <td><%= unit.incharge.try(:medic_name) %></td>
                             </tr>
                               <% end %>
                            </tbody>
                            </table>
                    </div>
                </div>

                <div class="span6">
                    <div class="slate">
                        <div class="page-header">
                            <h2><i class="icon-shopping-cart pull-right"></i>Units</h2>
                        </div>

                            <table class="table table-striped table-bordered">
                                <thead>
                                    <tr>
                                        <th>Unit</th>
                                        <th>Status</th>
                                    </tr>
                                </thead>
                                <tbody>
                                      <tr>
                                        <% @unit.each do |unit| %>
                                        <td><%= unit.try(:unit_name) %></td>
                                        <td><div class="<%= set_status(unit.status) %>"><%= unit.status.try(:unit_status) %></div></td>
                                        </tr>
                                        <% end %>
                                    </tbody>
                                </table>
                    </div>
                </div>
            </div>
        </div>      
    </div>
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Wow... yeah, that's a lot.

My immediate suggestion would be to simply make the dashboard do less. I doubt all of those things are of extreme importance all the time.

A lot seems like historical data (that spans years), but it's shown along side current stuff (that spans hours, it seems). I imagine most users only use a fraction of all that (and some users are no doubt intimidated). No offense intended, but I imagine the UI is simply overwhelming users - until they learn to ignore almost all of it, and just focus on the things they actually need.

If it must be on one page, load the less-important data on-demand via ajax.

Second suggestion: Aggressively cache as much as you can. Cache, cache, cache. For instance, any "YTD"-value will by definition only change once per day, not for each request. Check out the Rails Guides for some ideas and ActiveSupport::Cache for the implementation, and/or look into things like redis and memcached.

Rails has a lot built-in already, though. For instance, you could cache something like the last year's number of calls like so:

class Call
  def self.previous_year_cached
    expiry = Time.zone.now.end_of_year - Time.zone.now
    Rails.cache.fetch([self.name, "previous_year_count"], expires_in: expiry) do
      self.previous_year.count
    end
  end
end

So now, when you call Call.previous_year_cached it'll either give you a cached value without hitting the database, or it'll execute the block to find and store a new value. And it'll set the cache to expire on New Year's Eve (you could of course also just set to expire after 1.week or something, and skip the calculation, but it's just a little arithmetic).

Second line of caching is view caching. The Rails Guides I linked to above provide a good introduction to those. View caching will often give you even more of a speed-up, since rendering views is time-consuming. So view caching will give you the most bang for your buck, because you're caching at the very last step before sending the page to the browser. But any kind of caching will help speed things up, so data caching like above will also help. That way, even if a view has to be re-rendered, it might still avoid hitting the database by pulling its values from the cache.

You can also cache things without an explicit expiration date, and instead just flush (remove) the cache when it needs to update. For instance, you could cache today's calls, and flush the cached count when a new call record is added:

class Call
  after_create :flush_cache
  after_destroy :flush_cache

  def self.today_count_cached
    Rails.cache.fetch([self.name, "today_count"]) { self.today.count }
  end

  private

  def flush_cache
    Rails.cache.delete([self.class.name, "today_count"])
  end
end

You can of course add many more cached values this way, choosing when to store and delete them. See ActiveRecord's callbacks for the triggers you can act on.

Third option would be to do more client-side. Let users sort and filter instead of trying to anticipate every single data breakdown a user could want. Again, I doubt your users actually want all that data . They may think they do, but do they really? It's easy to say "yes" if we pretend there are no tradeoffs, but there are: usability and speed (and maintainability, and development time).

Try checking out ux.stackexchange.com. Besides perhaps finding some good tips for organizing a lot of data, you'll no doubt also find studies that indicate exactly how much information a human being can actually process. It's always useful to have scientific studies to refer to, if you want to argue for a more simple design.

I know it's trite, but less is more. Really.

I know this isn't much of a code review, but the individual pieces look OK on their own. There are just too many pieces, if you ask me. The only thing that looks iffy (after a quick glance) are all the "service level" scopes. If you just make a scope for every conceivable service level, you might as well just have 1 scope with a lambda, and pass in the service level (or use service_level.calls). What you have right now is overly specific, and couples everything very tightly.

\$\endgroup\$
8
  • \$\begingroup\$ Thank you for the feedback, I realize this dashboard is cluttered and needs to be refactored, especially from a UI/UX standpoint. I'm with you on creating 1 scope for service level. I'm honestly not sure how that would be written. And how I would pass a service level to the scope. I'm a bit rusty lol. I'm definitely going to look into caching my requests as I'm taking too much of a hit on the db when loading this resource. Believe it or not, my boss wants all of this data on the page. I wanted something much more simple and less cluttered, but he really wants all of those metrics. \$\endgroup\$
    – nulltek
    Aug 26, 2014 at 18:18
  • \$\begingroup\$ @cz3ch I figured someone had said they wanted all that stuff :) You can try explaining that the system can be better if you split things up; people are often sure that they want feature X, Y and Z all at once, but less sure if you explain the tradeoffs involved. And yes, cache like crazy. I'll add an example of view caching, when I have a moment. As for service level scope, you almost have it already in the by_service_level scope where you just pass a string. The other service level scopes are just syntactical sugar - not an inherently bad thing, but something you have to maintain \$\endgroup\$
    – Flambino
    Aug 27, 2014 at 6:23
  • \$\begingroup\$ @cz3ch Update my answer with a bunch of stuff about caching \$\endgroup\$
    – Flambino
    Aug 27, 2014 at 7:23
  • \$\begingroup\$ Thanks for the updated answer and the caching strategy. I'm definitely going to test this out and start caching YESTERDAY :) As for the service level scope, you're right. I can do the following Call.by_service_level('ALS').count to get a count by service level simply by passing a string. This is much better than having multiple service_level scopes and having to repeat myself. And yeah they wanted all of that stuff but I'm going to push for what's "ACTUALLY" needed versus what they want to see, all the while letting them know that it will perform and look better with less. \$\endgroup\$
    – nulltek
    Aug 27, 2014 at 19:06
  • \$\begingroup\$ @cz3ch Glad it was useful! And yeah, try to argue for some changes (responsibly of course; don't start a fight or anything) :) Just avoid the sentence "remove features" - people never want to hear that, regardless of good intentions. Besides, in this case, just moving some features (i.e. put them on separate pages) would no doubt help; but they're not going away. Perhaps doing so will even make room for new features. And everyone loves new features (or think that they do) ;) \$\endgroup\$
    – Flambino
    Aug 27, 2014 at 22:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.