2
\$\begingroup\$

I am working on a personal project and I have hit a wall. I know I am writing bad code and I really want to refactor the code below. The application has three tables on the same page. Each table contains data from a has-many-though relationship. In essence I have an employees page which contains three tables of employee licenses that all expire in grouped intervals:

ALL EMPLOYEE PAGE

  • Employees with licenses Expiring in 30 days
  • Employees with licenses Expiring in 30-90 days
  • Employees with licenses Expiring in 90 days

All three of these tables are independently paginated and I am allowing the user to enter a search term and search across all three tables.

However I have over 1200 licenses so the page is taking forever to load. How can I optimize this functionality? Any help would be greatly appreciated.

Model

  def self.emp_lic_small
   self.all.map{|se| se.employee_licenses.less_than_thirty}.flatten
  end

  def self.emp_lic_medium
    self.all.map{|se| se.employee_licenses.between_thirty_and_ninty}.flatten
  end

  def self.emp_lic_large
    self.all.map{|se| se.employee_licenses.greater_than_ninty}.flatten
  end

Controller

    @small_employee_licenses = SiteEmployee.search(params[:search]).emp_lic_small.paginate(:page => params[:small_lic], :per_page => 20)
    @medium_employee_licenses = SiteEmployee.search(params[:search]).emp_lic_medium.paginate(:page => params[:med_lic], :per_page => 20)
    @large_employee_licenses = SiteEmployee.search(params[:search]).emp_lic_large.paginate(:page => params[:large_lic], :per_page => 20)

View

<div class="panel panel-danger">
  <div class="panel-heading"><strong>Employee Licenses Expiring in Less Than 30 Days</strong></div>
    <table class="table">
        <thead>
            <th class="text-center">Employee Name</th>
            <th class="text-center">Employed By</th>
            <th class="text-center">License Name</th>
            <th class="text-center">Expiration Date</th>
            <th class="text-center">Obtained?</th>
            </tr>
        </thead>
        <tbody>
            <% if @small_employee_licenses.present? %>
                <% @small_employee_licenses.each do |e| %>
                    <tr>
                    <td class="text-center"><%= link_to e.site_employee.to_s, site_employee_path(e.site_employee)%></td>
                    <td class="text-center"><%= link_to e.site_employee.site.name, site_path(e.site_employee.site)%></td>
                    <td class="text-center"><%= e.license.name %></td>
                    <td class="text-center"><%= e.expiration_date.strftime("%m/%d/%Y") %></td>
                    <td class="text-center"><%= e.obtained? ? "Yes" : "No" %></td>
                    </tr>
                    <%end%>
            <% else %>
                <tr><td colspan="3">There are currently no Licenses due in the next 30 days.</td></tr>
            <% end %>
        </tbody>
    </table>
</div>
<%= will_paginate @small_employee_licenses, param_name:'small_lic' unless @small_employee_licenses.blank?  %> 

<div class="panel panel-warning">
  <div class="panel-heading"><strong>Employee Licenses Expiring in 30-90 Days</strong></div>
    <table class="table">
        <thead>
            <th class="text-center">Employee Name</th>
            <th class="text-center">Employed By</th>
            <th class="text-center">License Name</th>
            <th class="text-center">Expiration Date</th>
            <th class="text-center">Obtained?</th>
            </tr>
        </thead>
        <tbody>
            <% if @medium_employee_licenses.present? %>
                <% @medium_employee_licenses.each do |e| %>
                    <tr>
                    <td class="text-center"><%= link_to e.site_employee.to_s, site_employee_path(e.site_employee)%></td>
                    <td class="text-center"><%= link_to e.site_employee.site.name, site_path(e.site_employee.site)%></td>
                    <td class="text-center"><%= e.license.name %></td>
                    <td class="text-center"><%= e.expiration_date.strftime("%m/%d/%Y") %></td>
                    <td class="text-center"><%= e.obtained? ? "Yes" : "No" %></td>
                    </tr>
                    <%end%>
            <% else %>
                <tr><td colspan="3">There are currently no Licenses due in the next 30 days.</td></tr>
            <% end %>
        </tbody>
    </table>
</div>
<%= will_paginate @medium_employee_licenses, param_name:'med_lic' unless @medium_employee_licenses.blank?  %> 
<div class="panel panel-success">
  <div class="panel-heading"><strong>Employee Licenses Expiring in 30-90 Days</strong></div>
    <table class="table">
        <thead>
            <th class="text-center">Employee Name</th>
            <th class="text-center">Employed By</th>
            <th class="text-center">License Name</th>
            <th class="text-center">Expiration Date</th>
            <th class="text-center">Obtained?</th>
            </tr>
        </thead>
        <tbody>
            <% if @large_employee_licenses.present? %>
                <% @large_employee_licenses.each do |e| %>
                    <tr>
                    <td class="text-center"><%= link_to e.site_employee.to_s, site_employee_path(e.site_employee)%></td>
                    <td class="text-center"><%= link_to e.site_employee.site.name, site_path(e.site_employee.site)%></td>
                    <td class="text-center"><%= e.license.name %></td>
                    <td class="text-center"><%= e.expiration_date.strftime("%m/%d/%Y") %></td>
                    <td class="text-center"><%= e.obtained? ? "Yes" : "No" %></td>
                    </tr>
                    <%end%>
            <% else %>
                <tr><td colspan="3">There are currently no Licenses due in the next 30 days.</td></tr>
            <% end %>
        </tbody>
    </table>
</div>
<%= will_paginate @large_employee_licenses, param_name:'large_lic' unless @large_employee_licenses.blank?  %> 
\$\endgroup\$
4
  • \$\begingroup\$ I'd wager the bottleneck is in the model methods. You're mapping all records. I can't tell from the code exactly what you're scoping that to – what model is this from? And what's se? Are the lists scoped to something before searching? – but you're looking for licenses, but seem to be going the long way of finding licenses through something. It also implies that the pagination happens late - after a lot of stuff has been loaded. So the pagination is the act of throwing a lot of it away, rather than loading what you need. But your question doesn't have enough detail, so I'm not sure. \$\endgroup\$
    – Flambino
    Jan 8, 2017 at 19:41
  • \$\begingroup\$ Could you post how these tables are related? (the has_many, etc, for the tables in the example?) \$\endgroup\$
    – lcguida
    Jan 26, 2017 at 10:48
  • \$\begingroup\$ Do you really need all 1200 employees to be displayed before asking the user to search for one? Why not first ask for a search term and then to accordingly display results? \$\endgroup\$
    – BenKoshy
    Oct 8, 2017 at 5:49
  • \$\begingroup\$ @Flambino i think 'se' stands for SiteEmployee \$\endgroup\$
    – BenKoshy
    Oct 8, 2017 at 5:51

1 Answer 1

1
\$\begingroup\$

You should probably reduce the number of database queries, first you repeat the SiteEmployee search 3 times. Do it once and store it in a a variable. I would also query the database once with an order clause together with the Enumerable group_by method to get all of the results in one go.

\$\endgroup\$

Your Answer

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

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