Querying and rendering an HTML table of ISP performance statistics

Question

I have multiples measurements and I want to render it into tables like this

Measurements			Operator	browsings			FTP DL			FTP UL
Location	Event	Date	Operator	avg	min	max	avg	min	max	avg	min	max
			Verizon
			USCell
			T-Mobile

for each measurement rendered there is always 3 operator

the operator is fixed/determined value they are ["Verizon", "USCell", "T-Mobile"]

my current approach is to use multiple loop and using where clause to achieve this

measurements/index.html.erb

<table> <thead> <tr> <th colspan="3">Measurement</th> <th colspan="3">FTP DL</th> <th colspan="3">FTP UL</th> <th colspan="3">Browsing</th> </tr> <tr> <th>Location</th> <th>Event</th> <th>Date</th> <th>Average Browsing Speed (Mbps)</th> <th>Minimum Browsing Speed (Mbps)</th> <th>Maximum Browsing Speed (Mbps)</th> <th>Average FTP Download Speed (Mbps)</th> <th>Minimum FTP Download Speed (Mbps)</th> <th>Maximum FTP Download Speed (Mbps)</th> <th>Average FTP Upload Speed (Mbps)</th> <th>Minimum FTP Upload Speed (Mbps)</th> <th>Maximum FTP Upload Speed (Mbps)</th> </tr> </thead> <tbody> <% @measurements.each do |measurement| %> <% @operators.each do |operator| %> <tr> <td><%= measurement.location %></td> <td><%= measurement.event %></td> <td><%= measurement.date %></td> <td><%= operator %></td> <% if measurement.browsings.where(operator: operator).any? %> <td><%= measurement.browsings.where(operator: operator).first.avg %></td> <td><%= measurement.browsings.where(operator: operator).first.min %></td> <td><%= measurement.browsings.where(operator: operator).first.max %></td> <% else %> <td></td> <td></td> <td></td> <% end %> <% if measurement.ftp_dls.where(operator: operator).any? %> <td><%= measurement.ftp_dls.where(operator: operator).first.avg %></td> <td><%= measurement.ftp_dls.where(operator: operator).first.min %></td> <td><%= measurement.ftp_dls.where(operator: operator).first.max %></td> <% else %> <td></td> <td></td> <td></td> <td></td> <% end %> <% if measurement.ftp_uls.where(operator: operator).any? %> <td><%= measurement.ftp_uls.where(operator: operator).first.avg %></td> <td><%= measurement.ftp_uls.where(operator: operator).first.min %></td> <td><%= measurement.ftp_uls.where(operator: operator).first.max %></td> <% else %> <td></td> <td></td> <td></td> <% end %> </tr> <% end %> <% end %> </tbody> </table> 

I'm wondering if I could avoid using multiple loop and where clause on my code to achieve this?

because I think using multiple loop and where approach considered it as bad practices, isn't it?

I'm thinking since I have multiple repeated operator I could use that as a group or something, I'm thinking about joining all my has_many and group them by using group_by operator but I don't know how to do it yet (need a little guidance there if its really possible and good approach)

here are my others code

measurement.rb

class Measurement < ApplicationRecord has_many :ftp_uls, dependent: :destroy has_many :ftp_dls, dependent: :destroy has_many :browsings, dependent: :destroy end 

measurement_controller.rb

class MeasurementsController < ApplicationController before_action :authenticate_user!, only: [:index, :show] before_action :set_measurement, only: %i[ show edit update destroy ] def index @measurements = Measurement.all @operators = ["Verizon", "USCell", "T-Mobile"] end end 

schema.rb

ActiveRecord::Schema.define(version: 2021_08_17_062327) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" create_table "browsings", force: :cascade do |t| t.bigint "measurement_id", null: false t.string "operator" t.decimal "avg" t.decimal "min" t.decimal "max" t.index ["measurement_id"], name: "index_browsings_on_measurement_id" end create_table "ftp_dls", force: :cascade do |t| t.bigint "measurement_id", null: false t.string "operator" t.decimal "avg" t.decimal "min" t.decimal "max" t.index ["measurement_id"], name: "index_ftp_dls_on_measurement_id" end create_table "ftp_uls", force: :cascade do |t| t.bigint "measurement_id", null: false t.string "operator" t.decimal "avg" t.decimal "min" t.decimal "max" t.index ["measurement_id"], name: "index_ftp_uls_on_measurement_id" end create_table "measurements", force: :cascade do |t| t.string "location" t.string "event" t.date "date" end end 

Answer 1

So, the first problem I'd try to address is that you're running SQL queries in your views. This is generally considered a bad practice (and there are helpful gems/frameworks that will throw errors if you try to do this).

The second major issue is the # of sql queries being run, as we're executing multiple queries for each measurement (1+N problem). This is the source of your performance issue in both the original example and your updated code.

Ideally you want all the relevant data to be loaded in a small number of SQL queries ahead of time.

It looks like you want 1 row per measurement per operator (is that right?). This is not easy to do with your current database schema. I've added a sql query that builds this but it's complex. This could be a great use case for a SQL view with an activerecord object on top of it.

I'm also assuming you only have at most 1 row per measurement + operator combo in the browsings/ftp_dls/ftp_uls tables. If this is correct, I'd add a unique index to enforce this on each table. If there could be multiple rows per operator and measurement then you'll need to adjust the SQL or this will return duplicate rows.

So I'd start by building SQL queries that generates the data you want. Something like...

@operators = ["Verizon", "USCell", "T-Mobile"] # You don't need to hard-code the operators but I thought it made # the SQL easier to understand operator_join_sql = <<-SQL INNER JOIN (('Verizon'),('USCell'),('T-Mobile')) AS operators(name) ON true SQL browsings_join_sql = <<-SQL LEFT OUTER JOIN browsings ON browsings.operator = operators.name AND browsings.measurment_id = measurements.id SQL ftp_uls_join_sql = <<-SQL LEFT OUTER JOIN ftp_uls ON ftp_uls.operator = operators.name AND ftp_uls.measurment_id = measurements.id SQL ftp_dls_join_sql = <<-SQL LEFT OUTER JOIN ftp_dls ON ftp_dls.operator = operators.name AND ftp_dls.measurment_id = measurements.id SQL select_sql = <<-SQL SELECT measurements.* , operators.name AS operator , browsings.avg AS ftp_urls_avg , browsings.min AS ftp_urls_min , browsings.max AS ftp_urls_max , ftp_urls.avg AS ftp_urls_avg , ftp_urls.min AS ftp_urls_min , ftp_urls.max AS ftp_urls_max , ftp_drls.avg AS ftp_drls_avg , ftp_drls.min AS ftp_drls_min , ftp_drls.max AS ftp_drls_max SQL @measurement_by_operator = @measurements.select(select_sql) .joins(operator_join_sql) .joins(browsings_join_sql) .joins(ftp_uls_join_sql) .joins(ftp_dls_join_sql) 

Then, in your view, you would do:

<% @measurement_by_operator.each do |measurement| %> <tr> <td><%= measurement.location %></td> <td><%= measurement.event %></td> <td><%= measurement.date %></td> <td><%= measurement.operator %></td> <td><%= measurement.browsings_avg %></td> <td><%= measurement.browsings_min %></td> <td><%= measurement.browsings_max %></td> <td><%= measurement.ftp_uls_avg %></td> <td><%= measurement.ftp_uls_min %></td> <td><%= measurement.ftp_uls_max %></td> <td><%= measurement.ftp_dls_avg %></td> <td><%= measurement.ftp_dls_min %></td> <td><%= measurement.ftp_dls_max %></td> </ul> <% end %> 

---- EDIT ----

Some additional thoughts on your data schema...

Your three tables (browsings, ftp_uls, fpt_dls) have identical schema. They could be a single table (e.g. "measurement_readings" with a a "type" column, for example).

While the first approach I'm suggesting is probably best, it's fairly advanced. With the readings in a single table, there's an alternative that might be more approachable and still reasonable. Something like:

In your model (or better, a helper or presenter!)

class Measurement def for(operator, type) measurement_readings.detect{|mr| mr.operator == operator && mr.type == type } || NullMeasurementReading.new end end # A `NullMeasurementReading` is just a nullobject pattern. Could be a PORO or subclass of MeasurementReading or whatever. # Alternatively use `&.` in the view. # We're using `detect` here because we expect the array of readings to already be loaded # and explicitly *do not* want to trigger SQL queries 

your controller

@measurements = @measurements.preload(:measurement_readings) 

in the view

<% @measurements.each do |measurement| %> <% operators.each do |operator| %> <tr> <td><%= measurement.location %></td> <td><%= measurement.event %></td> <td><%= measurement.date %></td> <td><%= operator %></td> <td><%= measurement.for(operator, 'browsings').avg %> <td><%= measurement.for(operator, 'browsings').min %> <td><%= measurement.for(operator, 'browsings').max %> <%# etc.. %> </tr> <% end %> <% end %> 

Answer 2

there is many way to refactor this

first change operator into integer/enum instead string, or change operator to become an object/table instead with a name and order column

so that we can sort the browsings/ftpdl/ftp ul using operator column more easier

then to simplify the loop and query we can transform it into and array first then render the table row/data from that array

the new measurement_controller.rbindex method would be like this

def index @measurements = Measurement.all #we could also add eager loading here #Measurement.all.includes(:browsings, :ftp_dls, :ftp_uls) @rows = [] @measurements.each do |measurement| row = [] Operator.all.each_with_index do |operator, index| row << [measurement.location, measurement.event, measurement.date, operator.name] end measurement.browsings.each_with_index do |browsing, index| row[index].concat([browsing.avg, browsing.min, browsing.max]) end measurement.ftp_dls.each_with_index do |ftp_dl, index| row[index].concat([ftp_dl.avg, ftp_dl.min, ftp_dl.max]) end measurement.ftp_uls.each_with_index do |ftp_ul, index| row[index].concat([ftp_ul.avg, ftp_ul.min, ftp_ul.max]) end @rows << row end @rows = @rows.flatten(1) end 

the new measurements/index.html.erb<tbody> table render would be like this

<tbody> <% @rows.each do |row|%> <tr> <% row.each do |data| %> <td><%= data %></td> <% end %> </tr> <% end%> </tbody> 

I think it will perform faster than the code before and the code is more readable and also we avoid loop inside loop idk but people said loop inside loop is bad

I just test it and in my use case it reduce the load time from 266693.0 ms to 13103.0 ms and to 7432.4 ms with eager loading