Removing instance variable and before action (#16954)

There is no reason to create an instance variable as we don't pass this
to the view.  Further, by adding this as a before_action there's a
disconnect in logic.

This commit attempts to address those issues.

What follows is a three-fold change:

1. Removing the before action (let's just call the method)
2. Reworking the method to reduce, just a bit, the method cost
3. Removing an unused instance variable

Here are the benchmarks.  Note the "follows_limit" as written below is
the "Proposed" route.

```ruby
require 'benchmark'

def follows_limit_original(params:, default: 80, max: 1000)
  per_page = (params[:per_page] || default).to_i
  @follows_limit = [per_page, max].min
end

def follows_limit(params:, default: 80, max: 1000)
  return default unless params.key?(:per_page)
  per_page = params[:per_page].to_i
  return max

  per_page
end

def follows_limit_alt(params:, default: 80, max: 1000)
  per_page = params.fetch(:per_page, default).to_i
  return max if per_page > max
  per_page
end

TIMES = 10_000
Benchmark.bmbm do |b|
  b.report("Original no params") { 1000.times { follows_limit_original(params: {}) } }
  b.report("Original less than max") { 1000.times { follows_limit_original(params: {per_page: 90 }) } }
  b.report("Original greater than max") { 1000.times { follows_limit_original(params: {per_page: 9000 }) } }
  b.report("Proposed no params") { 1000.times { follows_limit(params: {}) } }
  b.report("Proposed less than max") { 1000.times { follows_limit(params: {per_page: 90 }) } }
  b.report("Proposed greater than max") { 1000.times { follows_limit(params: {per_page: 9000 }) } }
  b.report("Alt no params") { 1000.times { follows_limit_alt(params: {}) } }
  b.report("Alt less than max") { 1000.times { follows_limit_alt(params: {per_page: 90 }) } }
  b.report("Alt greater than max") { 1000.times { follows_limit_alt(params: {per_page: 9000 }) } }
end
```

```shell
$ ruby /Users/jfriesen/git/forem/bench.rb
Rehearsal -------------------------------------------------------------
Original no params          0.000403   0.000002   0.000405 (  0.000405)
Original less than max      0.000109   0.000005   0.000114 (  0.000114)
Original greater than max   0.000161   0.000006   0.000167 (  0.000166)
Proposed no params          0.000076   0.000001   0.000077 (  0.000076)
Proposed less than max      0.000114   0.000009   0.000123 (  0.000126)
Proposed greater than max   0.000112   0.000011   0.000123 (  0.000123)
Alt no params               0.000104   0.000007   0.000111 (  0.000114)
Alt less than max           0.000113   0.000009   0.000122 (  0.000122)
Alt greater than max        0.000111   0.000001   0.000112 (  0.000115)
---------------------------------------------------- total: 0.001354sec

                                user     system      total        real
Original no params          0.000108   0.000000   0.000108 (  0.000110)
Original less than max      0.000109   0.000001   0.000110 (  0.000111)
Original greater than max   0.000109   0.000000   0.000109 (  0.000109)
Proposed no params          0.000071   0.000000   0.000071 (  0.000071)
Proposed less than max      0.000103   0.000001   0.000104 (  0.000103)
Proposed greater than max   0.000102   0.000000   0.000102 (  0.000103)
Alt no params               0.000093   0.000000   0.000093 (  0.000093)
Alt less than max           0.000103   0.000000   0.000103 (  0.000104)
Alt greater than max        0.000102   0.000000   0.000102 (  0.000103)
```
This commit is contained in:
Jeremy Friesen 2022-03-22 12:57:57 -04:00 committed by GitHub
parent 5fab2f86b0
commit 73ff4169e8
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23

View file

@ -1,8 +1,9 @@
class DashboardsController < ApplicationController
before_action :set_no_cache_header
before_action :authenticate_user!
before_action -> { limit_per_page(default: 80, max: 1000) }, except: %i[show analytics]
LIMIT_PER_PAGE_DEFAULT = 80
LIMIT_PER_PAGE_MAX = 1000
def show
fetch_and_authorize_user
target = @user
@ -30,31 +31,31 @@ class DashboardsController < ApplicationController
def following_tags
fetch_and_authorize_user
@followed_tags = @user.follows_by_type("ActsAsTaggableOn::Tag")
.order(points: :desc).includes(:followable).limit(@follows_limit)
.order(points: :desc).includes(:followable).limit(follows_limit)
end
def following_users
fetch_and_authorize_user
@follows = @user.follows_by_type("User")
.order(created_at: :desc).includes(:followable).limit(@follows_limit)
.order(created_at: :desc).includes(:followable).limit(follows_limit)
end
def following_organizations
fetch_and_authorize_user
@followed_organizations = @user.follows_by_type("Organization")
.order(created_at: :desc).includes(:followable).limit(@follows_limit)
.order(created_at: :desc).includes(:followable).limit(follows_limit)
end
def following_podcasts
fetch_and_authorize_user
@followed_podcasts = @user.follows_by_type("Podcast")
.order(created_at: :desc).includes(:followable).limit(@follows_limit)
.order(created_at: :desc).includes(:followable).limit(follows_limit)
end
def followers
fetch_and_authorize_user
@follows = Follow.followable_user(@user.id)
.includes(:follower).order(created_at: :desc).limit(@follows_limit)
.includes(:follower).order(created_at: :desc).limit(follows_limit)
end
def analytics
@ -94,8 +95,12 @@ class DashboardsController < ApplicationController
authorize (@user || User), :dashboard_show?
end
def limit_per_page(default:, max:)
per_page = (params[:per_page] || default).to_i
@follows_limit = [per_page, max].min
def follows_limit(default: LIMIT_PER_PAGE_DEFAULT, max: LIMIT_PER_PAGE_MAX)
return default unless params.key?(:per_page)
per_page = params[:per_page].to_i
return max if per_page > max
per_page
end
end