TracksApp/tracks

View on GitHub
app/controllers/users_controller.rb

Summary

Maintainability
A
3 hrs
Test Coverage

Unprotected mass assignment
Open

        user = User.new(user_params)

Mass assignment is a feature of Rails which allows an application to create a record from the values of a hash.

Example:

User.new(params[:user])

Unfortunately, if there is a user field called admin which controls administrator access, now any user can make themselves an administrator.

attr_accessible and attr_protected can be used to limit mass assignment. However, Brakeman will warn unless attr_accessible is used, or mass assignment is completely disabled.

There are two different mass assignment warnings which can arise. The first is when mass assignment actually occurs, such as the example above. This results in a warning like

Unprotected mass assignment near line 61: User.new(params[:user])

The other warning is raised whenever a model is found which does not use attr_accessible. This produces generic warnings like

Mass assignment is not restricted using attr_accessible

with a list of affected models.

In Rails 3.1 and newer, mass assignment can easily be disabled:

config.active_record.whitelist_attributes = true

Unfortunately, it can also easily be bypassed:

User.new(params[:user], :without_protection => true)

Brakeman will warn on uses of without_protection.

Unprotected mass assignment
Open

        user = User.new(user_params)

Mass assignment is a feature of Rails which allows an application to create a record from the values of a hash.

Example:

User.new(params[:user])

Unfortunately, if there is a user field called admin which controls administrator access, now any user can make themselves an administrator.

attr_accessible and attr_protected can be used to limit mass assignment. However, Brakeman will warn unless attr_accessible is used, or mass assignment is completely disabled.

There are two different mass assignment warnings which can arise. The first is when mass assignment actually occurs, such as the example above. This results in a warning like

Unprotected mass assignment near line 61: User.new(params[:user])

The other warning is raised whenever a model is found which does not use attr_accessible. This produces generic warnings like

Mass assignment is not restricted using attr_accessible

with a list of affected models.

In Rails 3.1 and newer, mass assignment can easily be disabled:

config.active_record.whitelist_attributes = true

Unfortunately, it can also easily be bypassed:

User.new(params[:user], :without_protection => true)

Brakeman will warn on uses of without_protection.

Complex method UsersController#create (108.6)
Open

  def create
    if params['exception']
      render_failure "Expected post format is valid xml like so: <user><login>username</login><password>abc123</password></user>."
      return
    end
Severity: Minor
Found in app/controllers/users_controller.rb by flog

Flog calculates the ABC score for methods. The ABC score is based on assignments, branches (method calls), and conditions.

You can read more about ABC metrics or the flog tool

Block has too many lines. [52/25]
Open

    respond_to do |format|
      format.html do
        unless User.no_users_yet? || (@user && @user.is_admin?) || SITE_CONFIG['open_signups']
          @page_title = t('users.no_signups_title')
          @admin_email = SITE_CONFIG['admin_email']
Severity: Minor
Found in app/controllers/users_controller.rb by rubocop

This cop checks if the length of a block exceeds some maximum value. Comment lines can optionally be ignored. The maximum allowed length is configurable. The cop can be configured to ignore blocks passed to certain methods.

Complex method UsersController#destroy (36.3)
Open

  def destroy
    @deleted_user = User.find(params[:id])

    # Remove the user
    @saved = @deleted_user.destroy
Severity: Minor
Found in app/controllers/users_controller.rb by flog

Flog calculates the ABC score for methods. The ABC score is based on assignments, branches (method calls), and conditions.

You can read more about ABC metrics or the flog tool

Complex method UsersController#index (33.3)
Open

  def index
    respond_to do |format|
      order_by = 'login'
      if params[:order] && User.column_names.include?(params[:order])
        order_by = params[:order]
Severity: Minor
Found in app/controllers/users_controller.rb by flog

Flog calculates the ABC score for methods. The ABC score is based on assignments, branches (method calls), and conditions.

You can read more about ABC metrics or the flog tool

Complex method UsersController#new (26.2)
Open

  def new
    @auth_types = []
    unless session[:cas_user]
      Tracks::Config.auth_schemes.each { |auth| @auth_types << [auth, auth] }
    else
Severity: Minor
Found in app/controllers/users_controller.rb by flog

Flog calculates the ABC score for methods. The ABC score is based on assignments, branches (method calls), and conditions.

You can read more about ABC metrics or the flog tool

UsersController has at least 9 instance variables
Open

class UsersController < ApplicationController
Severity: Minor
Found in app/controllers/users_controller.rb by reek

Too Many Instance Variables is a special case of LargeClass.

Example

Given this configuration

TooManyInstanceVariables:
  max_instance_variables: 3

and this code:

class TooManyInstanceVariables
  def initialize
    @arg_1 = :dummy
    @arg_2 = :dummy
    @arg_3 = :dummy
    @arg_4 = :dummy
  end
end

Reek would emit the following warning:

test.rb -- 5 warnings:
  [1]:TooManyInstanceVariables has at least 4 instance variables (TooManyInstanceVariables)

UsersController#create has approx 36 statements
Open

  def create
Severity: Minor
Found in app/controllers/users_controller.rb by reek

A method with Too Many Statements is any method that has a large number of lines.

Too Many Statements warns about any method that has more than 5 statements. Reek's smell detector for Too Many Statements counts +1 for every simple statement in a method and +1 for every statement within a control structure (if, else, case, when, for, while, until, begin, rescue) but it doesn't count the control structure itself.

So the following method would score +6 in Reek's statement-counting algorithm:

def parse(arg, argv, &error)
  if !(val = arg) and (argv.empty? or /\A-/ =~ (val = argv[0]))
    return nil, block, nil                                         # +1
  end
  opt = (val = parse_arg(val, &error))[1]                          # +2
  val = conv_arg(*val)                                             # +3
  if opt and !arg
    argv.shift                                                     # +4
  else
    val[0] = nil                                                   # +5
  end
  val                                                              # +6
end

(You might argue that the two assigments within the first @if@ should count as statements, and that perhaps the nested assignment should count as +2.)

UsersController#update_password has approx 6 statements
Open

  def update_password
Severity: Minor
Found in app/controllers/users_controller.rb by reek

A method with Too Many Statements is any method that has a large number of lines.

Too Many Statements warns about any method that has more than 5 statements. Reek's smell detector for Too Many Statements counts +1 for every simple statement in a method and +1 for every statement within a control structure (if, else, case, when, for, while, until, begin, rescue) but it doesn't count the control structure itself.

So the following method would score +6 in Reek's statement-counting algorithm:

def parse(arg, argv, &error)
  if !(val = arg) and (argv.empty? or /\A-/ =~ (val = argv[0]))
    return nil, block, nil                                         # +1
  end
  opt = (val = parse_arg(val, &error))[1]                          # +2
  val = conv_arg(*val)                                             # +3
  if opt and !arg
    argv.shift                                                     # +4
  else
    val[0] = nil                                                   # +5
  end
  val                                                              # +6
end

(You might argue that the two assigments within the first @if@ should count as statements, and that perhaps the nested assignment should count as +2.)

UsersController#new has approx 15 statements
Open

  def new
Severity: Minor
Found in app/controllers/users_controller.rb by reek

A method with Too Many Statements is any method that has a large number of lines.

Too Many Statements warns about any method that has more than 5 statements. Reek's smell detector for Too Many Statements counts +1 for every simple statement in a method and +1 for every statement within a control structure (if, else, case, when, for, while, until, begin, rescue) but it doesn't count the control structure itself.

So the following method would score +6 in Reek's statement-counting algorithm:

def parse(arg, argv, &error)
  if !(val = arg) and (argv.empty? or /\A-/ =~ (val = argv[0]))
    return nil, block, nil                                         # +1
  end
  opt = (val = parse_arg(val, &error))[1]                          # +2
  val = conv_arg(*val)                                             # +3
  if opt and !arg
    argv.shift                                                     # +4
  else
    val[0] = nil                                                   # +5
  end
  val                                                              # +6
end

(You might argue that the two assigments within the first @if@ should count as statements, and that perhaps the nested assignment should count as +2.)

UsersController#index has approx 11 statements
Open

  def index
Severity: Minor
Found in app/controllers/users_controller.rb by reek

A method with Too Many Statements is any method that has a large number of lines.

Too Many Statements warns about any method that has more than 5 statements. Reek's smell detector for Too Many Statements counts +1 for every simple statement in a method and +1 for every statement within a control structure (if, else, case, when, for, while, until, begin, rescue) but it doesn't count the control structure itself.

So the following method would score +6 in Reek's statement-counting algorithm:

def parse(arg, argv, &error)
  if !(val = arg) and (argv.empty? or /\A-/ =~ (val = argv[0]))
    return nil, block, nil                                         # +1
  end
  opt = (val = parse_arg(val, &error))[1]                          # +2
  val = conv_arg(*val)                                             # +3
  if opt and !arg
    argv.shift                                                     # +4
  else
    val[0] = nil                                                   # +5
  end
  val                                                              # +6
end

(You might argue that the two assigments within the first @if@ should count as statements, and that perhaps the nested assignment should count as +2.)

UsersController#check_create_user_params has approx 6 statements
Open

  def check_create_user_params
Severity: Minor
Found in app/controllers/users_controller.rb by reek

A method with Too Many Statements is any method that has a large number of lines.

Too Many Statements warns about any method that has more than 5 statements. Reek's smell detector for Too Many Statements counts +1 for every simple statement in a method and +1 for every statement within a control structure (if, else, case, when, for, while, until, begin, rescue) but it doesn't count the control structure itself.

So the following method would score +6 in Reek's statement-counting algorithm:

def parse(arg, argv, &error)
  if !(val = arg) and (argv.empty? or /\A-/ =~ (val = argv[0]))
    return nil, block, nil                                         # +1
  end
  opt = (val = parse_arg(val, &error))[1]                          # +2
  val = conv_arg(*val)                                             # +3
  if opt and !arg
    argv.shift                                                     # +4
  else
    val[0] = nil                                                   # +5
  end
  val                                                              # +6
end

(You might argue that the two assigments within the first @if@ should count as statements, and that perhaps the nested assignment should count as +2.)

UsersController#destroy has approx 13 statements
Open

  def destroy
Severity: Minor
Found in app/controllers/users_controller.rb by reek

A method with Too Many Statements is any method that has a large number of lines.

Too Many Statements warns about any method that has more than 5 statements. Reek's smell detector for Too Many Statements counts +1 for every simple statement in a method and +1 for every statement within a control structure (if, else, case, when, for, while, until, begin, rescue) but it doesn't count the control structure itself.

So the following method would score +6 in Reek's statement-counting algorithm:

def parse(arg, argv, &error)
  if !(val = arg) and (argv.empty? or /\A-/ =~ (val = argv[0]))
    return nil, block, nil                                         # +1
  end
  opt = (val = parse_arg(val, &error))[1]                          # +2
  val = conv_arg(*val)                                             # +3
  if opt and !arg
    argv.shift                                                     # +4
  else
    val[0] = nil                                                   # +5
  end
  val                                                              # +6
end

(You might argue that the two assigments within the first @if@ should count as statements, and that perhaps the nested assignment should count as +2.)

Complex method UsersController#update_auth_type (25.0)
Open

  def update_auth_type
    current_user.auth_type = user_params[:auth_type]
    if current_user.save
      notify :notice, t('users.auth_type_updated')
      redirect_to preferences_path
Severity: Minor
Found in app/controllers/users_controller.rb by flog

Flog calculates the ABC score for methods. The ABC score is based on assignments, branches (method calls), and conditions.

You can read more about ABC metrics or the flog tool

UsersController#create calls '@user.is_admin?' 2 times
Open

        unless User.no_users_yet? || (@user && @user.is_admin?) || SITE_CONFIG['open_signups']
          @page_title = t('users.no_signups_title')
          @admin_email = SITE_CONFIG['admin_email']
          render :action => "nosignup", :layout => "login"
          return
Severity: Minor
Found in app/controllers/users_controller.rb by reek

Duplication occurs when two fragments of code look nearly identical, or when two fragments of code have nearly identical effects at some conceptual level.

Reek implements a check for Duplicate Method Call.

Example

Here's a very much simplified and contrived example. The following method will report a warning:

def double_thing()
  @other.thing + @other.thing
end

One quick approach to silence Reek would be to refactor the code thus:

def double_thing()
  thing = @other.thing
  thing + thing
end

A slightly different approach would be to replace all calls of double_thing by calls to @other.double_thing:

class Other
  def double_thing()
    thing + thing
  end
end

The approach you take will depend on balancing other factors in your code.

UsersController#create calls 'render_failure "You have to accept the terms of service to sign up!"' 2 times
Open

          render_failure "You have to accept the terms of service to sign up!"
          return
        end

        user = User.new(user_params)
Severity: Minor
Found in app/controllers/users_controller.rb by reek

Duplication occurs when two fragments of code look nearly identical, or when two fragments of code have nearly identical effects at some conceptual level.

Reek implements a check for Duplicate Method Call.

Example

Here's a very much simplified and contrived example. The following method will report a warning:

def double_thing()
  @other.thing + @other.thing
end

One quick approach to silence Reek would be to refactor the code thus:

def double_thing()
  thing = @other.thing
  thing + thing
end

A slightly different approach would be to replace all calls of double_thing by calls to @other.double_thing:

class Other
  def double_thing()
    thing + thing
  end
end

The approach you take will depend on balancing other factors in your code.

UsersController assumes too much for instance variable '@deleted_user'
Open

class UsersController < ApplicationController
Severity: Minor
Found in app/controllers/users_controller.rb by reek

Classes should not assume that instance variables are set or present outside of the current class definition.

Good:

class Foo
  def initialize
    @bar = :foo
  end

  def foo?
    @bar == :foo
  end
end

Good as well:

class Foo
  def foo?
    bar == :foo
  end

  def bar
    @bar ||= :foo
  end
end

Bad:

class Foo
  def go_foo!
    @bar = :foo
  end

  def foo?
    @bar == :foo
  end
end

Example

Running Reek on:

class Dummy
  def test
    @ivar
  end
end

would report:

[1]:InstanceVariableAssumption: Dummy assumes too much for instance variable @ivar

Note that this example would trigger this smell warning as well:

class Parent
  def initialize(omg)
    @omg = omg
  end
end

class Child < Parent
  def foo
    @omg
  end
end

The way to address the smell warning is that you should create an attr_reader to use @omg in the subclass and not access @omg directly like this:

class Parent
  attr_reader :omg

  def initialize(omg)
    @omg = omg
  end
end

class Child < Parent
  def foo
    omg
  end
end

Directly accessing instance variables is considered a smell because it breaks encapsulation and makes it harder to reason about code.

If you don't want to expose those methods as public API just make them private like this:

class Parent
  def initialize(omg)
    @omg = omg
  end

  private
  attr_reader :omg
end

class Child < Parent
  def foo
    omg
  end
end

Current Support in Reek

An instance variable must:

  • be set in the constructor
  • or be accessed through a method with lazy initialization / memoization.

If not, Instance Variable Assumption will be reported.

UsersController#destroy calls 'current_user == @deleted_user' 2 times
Open

    if @saved && current_user == @deleted_user
      logout_user
    end

    respond_to do |format|
Severity: Minor
Found in app/controllers/users_controller.rb by reek

Duplication occurs when two fragments of code look nearly identical, or when two fragments of code have nearly identical effects at some conceptual level.

Reek implements a check for Duplicate Method Call.

Example

Here's a very much simplified and contrived example. The following method will report a warning:

def double_thing()
  @other.thing + @other.thing
end

One quick approach to silence Reek would be to refactor the code thus:

def double_thing()
  thing = @other.thing
  thing + thing
end

A slightly different approach would be to replace all calls of double_thing by calls to @other.double_thing:

class Other
  def double_thing()
    thing + thing
  end
end

The approach you take will depend on balancing other factors in your code.

UsersController#destroy calls '@deleted_user.login' 2 times
Open

          notify :notice, t('users.successfully_deleted_user', :username => @deleted_user.login)
        else
          notify :error, t('users.failed_to_delete_user', :username => @deleted_user.login)
Severity: Minor
Found in app/controllers/users_controller.rb by reek

Duplication occurs when two fragments of code look nearly identical, or when two fragments of code have nearly identical effects at some conceptual level.

Reek implements a check for Duplicate Method Call.

Example

Here's a very much simplified and contrived example. The following method will report a warning:

def double_thing()
  @other.thing + @other.thing
end

One quick approach to silence Reek would be to refactor the code thus:

def double_thing()
  thing = @other.thing
  thing + thing
end

A slightly different approach would be to replace all calls of double_thing by calls to @other.double_thing:

class Other
  def double_thing()
    thing + thing
  end
end

The approach you take will depend on balancing other factors in your code.

UsersController assumes too much for instance variable '@saved'
Open

class UsersController < ApplicationController
Severity: Minor
Found in app/controllers/users_controller.rb by reek

Classes should not assume that instance variables are set or present outside of the current class definition.

Good:

class Foo
  def initialize
    @bar = :foo
  end

  def foo?
    @bar == :foo
  end
end

Good as well:

class Foo
  def foo?
    bar == :foo
  end

  def bar
    @bar ||= :foo
  end
end

Bad:

class Foo
  def go_foo!
    @bar = :foo
  end

  def foo?
    @bar == :foo
  end
end

Example

Running Reek on:

class Dummy
  def test
    @ivar
  end
end

would report:

[1]:InstanceVariableAssumption: Dummy assumes too much for instance variable @ivar

Note that this example would trigger this smell warning as well:

class Parent
  def initialize(omg)
    @omg = omg
  end
end

class Child < Parent
  def foo
    @omg
  end
end

The way to address the smell warning is that you should create an attr_reader to use @omg in the subclass and not access @omg directly like this:

class Parent
  attr_reader :omg

  def initialize(omg)
    @omg = omg
  end
end

class Child < Parent
  def foo
    omg
  end
end

Directly accessing instance variables is considered a smell because it breaks encapsulation and makes it harder to reason about code.

If you don't want to expose those methods as public API just make them private like this:

class Parent
  def initialize(omg)
    @omg = omg
  end

  private
  attr_reader :omg
end

class Child < Parent
  def foo
    omg
  end
end

Current Support in Reek

An instance variable must:

  • be set in the constructor
  • or be accessed through a method with lazy initialization / memoization.

If not, Instance Variable Assumption will be reported.

UsersController#check_create_user_params calls 'params[:user]' 4 times
Open

    return false unless params[:user].key?(:login)
    return false if params[:user][:login].empty?
    return false unless params[:user].key?(:password)
    return false if params[:user][:password].empty?
Severity: Minor
Found in app/controllers/users_controller.rb by reek

Duplication occurs when two fragments of code look nearly identical, or when two fragments of code have nearly identical effects at some conceptual level.

Reek implements a check for Duplicate Method Call.

Example

Here's a very much simplified and contrived example. The following method will report a warning:

def double_thing()
  @other.thing + @other.thing
end

One quick approach to silence Reek would be to refactor the code thus:

def double_thing()
  thing = @other.thing
  thing + thing
end

A slightly different approach would be to replace all calls of double_thing by calls to @other.double_thing:

class Other
  def double_thing()
    thing + thing
  end
end

The approach you take will depend on balancing other factors in your code.

UsersController#create calls 'SITE_CONFIG['tos_link'].blank?' 2 times
Open

        unless params['approve_tos'] == 'on' || SITE_CONFIG['tos_link'].blank?
          render_failure "You have to accept the terms of service to sign up!"
          return
        end

Severity: Minor
Found in app/controllers/users_controller.rb by reek

Duplication occurs when two fragments of code look nearly identical, or when two fragments of code have nearly identical effects at some conceptual level.

Reek implements a check for Duplicate Method Call.

Example

Here's a very much simplified and contrived example. The following method will report a warning:

def double_thing()
  @other.thing + @other.thing
end

One quick approach to silence Reek would be to refactor the code thus:

def double_thing()
  thing = @other.thing
  thing + thing
end

A slightly different approach would be to replace all calls of double_thing by calls to @other.double_thing:

class Other
  def double_thing()
    thing + thing
  end
end

The approach you take will depend on balancing other factors in your code.

UsersController#create calls 'user.save' 2 times
Open

        if user.save
          @user = User.authenticate(user.login, params['user']['password'])
          @user.create_preference(:locale => I18n.locale)
          @user.save
          session['user_id'] = @user.id unless signup_by_admin
Severity: Minor
Found in app/controllers/users_controller.rb by reek

Duplication occurs when two fragments of code look nearly identical, or when two fragments of code have nearly identical effects at some conceptual level.

Reek implements a check for Duplicate Method Call.

Example

Here's a very much simplified and contrived example. The following method will report a warning:

def double_thing()
  @other.thing + @other.thing
end

One quick approach to silence Reek would be to refactor the code thus:

def double_thing()
  thing = @other.thing
  thing + thing
end

A slightly different approach would be to replace all calls of double_thing by calls to @other.double_thing:

class Other
  def double_thing()
    thing + thing
  end
end

The approach you take will depend on balancing other factors in your code.

UsersController#create calls 'SITE_CONFIG['tos_link']' 2 times
Open

        unless params['approve_tos'] == 'on' || SITE_CONFIG['tos_link'].blank?
          render_failure "You have to accept the terms of service to sign up!"
          return
        end

Severity: Minor
Found in app/controllers/users_controller.rb by reek

Duplication occurs when two fragments of code look nearly identical, or when two fragments of code have nearly identical effects at some conceptual level.

Reek implements a check for Duplicate Method Call.

Example

Here's a very much simplified and contrived example. The following method will report a warning:

def double_thing()
  @other.thing + @other.thing
end

One quick approach to silence Reek would be to refactor the code thus:

def double_thing()
  thing = @other.thing
  thing + thing
end

A slightly different approach would be to replace all calls of double_thing by calls to @other.double_thing:

class Other
  def double_thing()
    thing + thing
  end
end

The approach you take will depend on balancing other factors in your code.

UsersController#index calls 'params[:order]' 3 times
Open

      if params[:order] && User.column_names.include?(params[:order])
        order_by = params[:order]
Severity: Minor
Found in app/controllers/users_controller.rb by reek

Duplication occurs when two fragments of code look nearly identical, or when two fragments of code have nearly identical effects at some conceptual level.

Reek implements a check for Duplicate Method Call.

Example

Here's a very much simplified and contrived example. The following method will report a warning:

def double_thing()
  @other.thing + @other.thing
end

One quick approach to silence Reek would be to refactor the code thus:

def double_thing()
  thing = @other.thing
  thing + thing
end

A slightly different approach would be to replace all calls of double_thing by calls to @other.double_thing:

class Other
  def double_thing()
    thing + thing
  end
end

The approach you take will depend on balancing other factors in your code.

UsersController assumes too much for instance variable '@users'
Open

class UsersController < ApplicationController
Severity: Minor
Found in app/controllers/users_controller.rb by reek

Classes should not assume that instance variables are set or present outside of the current class definition.

Good:

class Foo
  def initialize
    @bar = :foo
  end

  def foo?
    @bar == :foo
  end
end

Good as well:

class Foo
  def foo?
    bar == :foo
  end

  def bar
    @bar ||= :foo
  end
end

Bad:

class Foo
  def go_foo!
    @bar = :foo
  end

  def foo?
    @bar == :foo
  end
end

Example

Running Reek on:

class Dummy
  def test
    @ivar
  end
end

would report:

[1]:InstanceVariableAssumption: Dummy assumes too much for instance variable @ivar

Note that this example would trigger this smell warning as well:

class Parent
  def initialize(omg)
    @omg = omg
  end
end

class Child < Parent
  def foo
    @omg
  end
end

The way to address the smell warning is that you should create an attr_reader to use @omg in the subclass and not access @omg directly like this:

class Parent
  attr_reader :omg

  def initialize(omg)
    @omg = omg
  end
end

class Child < Parent
  def foo
    omg
  end
end

Directly accessing instance variables is considered a smell because it breaks encapsulation and makes it harder to reason about code.

If you don't want to expose those methods as public API just make them private like this:

class Parent
  def initialize(omg)
    @omg = omg
  end

  private
  attr_reader :omg
end

class Child < Parent
  def foo
    omg
  end
end

Current Support in Reek

An instance variable must:

  • be set in the constructor
  • or be accessed through a method with lazy initialization / memoization.

If not, Instance Variable Assumption will be reported.

UsersController#create calls 'User.no_users_yet?' 2 times
Open

        unless User.no_users_yet? || (@user && @user.is_admin?) || SITE_CONFIG['open_signups']
          @page_title = t('users.no_signups_title')
          @admin_email = SITE_CONFIG['admin_email']
          render :action => "nosignup", :layout => "login"
          return
Severity: Minor
Found in app/controllers/users_controller.rb by reek

Duplication occurs when two fragments of code look nearly identical, or when two fragments of code have nearly identical effects at some conceptual level.

Reek implements a check for Duplicate Method Call.

Example

Here's a very much simplified and contrived example. The following method will report a warning:

def double_thing()
  @other.thing + @other.thing
end

One quick approach to silence Reek would be to refactor the code thus:

def double_thing()
  thing = @other.thing
  thing + thing
end

A slightly different approach would be to replace all calls of double_thing by calls to @other.double_thing:

class Other
  def double_thing()
    thing + thing
  end
end

The approach you take will depend on balancing other factors in your code.

UsersController has no descriptive comment
Open

class UsersController < ApplicationController
Severity: Minor
Found in app/controllers/users_controller.rb by reek

Classes and modules are the units of reuse and release. It is therefore considered good practice to annotate every class and module with a brief comment outlining its responsibilities.

Example

Given

class Dummy
  # Do things...
end

Reek would emit the following warning:

test.rb -- 1 warning:
  [1]:Dummy has no descriptive comment (IrresponsibleModule)

Fixing this is simple - just an explaining comment:

# The Dummy class is responsible for ...
class Dummy
  # Do things...
end

UsersController assumes too much for instance variable '@user'
Open

class UsersController < ApplicationController
Severity: Minor
Found in app/controllers/users_controller.rb by reek

Classes should not assume that instance variables are set or present outside of the current class definition.

Good:

class Foo
  def initialize
    @bar = :foo
  end

  def foo?
    @bar == :foo
  end
end

Good as well:

class Foo
  def foo?
    bar == :foo
  end

  def bar
    @bar ||= :foo
  end
end

Bad:

class Foo
  def go_foo!
    @bar = :foo
  end

  def foo?
    @bar == :foo
  end
end

Example

Running Reek on:

class Dummy
  def test
    @ivar
  end
end

would report:

[1]:InstanceVariableAssumption: Dummy assumes too much for instance variable @ivar

Note that this example would trigger this smell warning as well:

class Parent
  def initialize(omg)
    @omg = omg
  end
end

class Child < Parent
  def foo
    @omg
  end
end

The way to address the smell warning is that you should create an attr_reader to use @omg in the subclass and not access @omg directly like this:

class Parent
  attr_reader :omg

  def initialize(omg)
    @omg = omg
  end
end

class Child < Parent
  def foo
    omg
  end
end

Directly accessing instance variables is considered a smell because it breaks encapsulation and makes it harder to reason about code.

If you don't want to expose those methods as public API just make them private like this:

class Parent
  def initialize(omg)
    @omg = omg
  end

  private
  attr_reader :omg
end

class Child < Parent
  def foo
    omg
  end
end

Current Support in Reek

An instance variable must:

  • be set in the constructor
  • or be accessed through a method with lazy initialization / memoization.

If not, Instance Variable Assumption will be reported.

UsersController#get_new_user calls 'session['new_user']' 2 times
Open

    if session['new_user']
      user = session['new_user']
Severity: Minor
Found in app/controllers/users_controller.rb by reek

Duplication occurs when two fragments of code look nearly identical, or when two fragments of code have nearly identical effects at some conceptual level.

Reek implements a check for Duplicate Method Call.

Example

Here's a very much simplified and contrived example. The following method will report a warning:

def double_thing()
  @other.thing + @other.thing
end

One quick approach to silence Reek would be to refactor the code thus:

def double_thing()
  thing = @other.thing
  thing + thing
end

A slightly different approach would be to replace all calls of double_thing by calls to @other.double_thing:

class Other
  def double_thing()
    thing + thing
  end
end

The approach you take will depend on balancing other factors in your code.

UsersController assumes too much for instance variable '@auth_types'
Open

class UsersController < ApplicationController
Severity: Minor
Found in app/controllers/users_controller.rb by reek

Classes should not assume that instance variables are set or present outside of the current class definition.

Good:

class Foo
  def initialize
    @bar = :foo
  end

  def foo?
    @bar == :foo
  end
end

Good as well:

class Foo
  def foo?
    bar == :foo
  end

  def bar
    @bar ||= :foo
  end
end

Bad:

class Foo
  def go_foo!
    @bar = :foo
  end

  def foo?
    @bar == :foo
  end
end

Example

Running Reek on:

class Dummy
  def test
    @ivar
  end
end

would report:

[1]:InstanceVariableAssumption: Dummy assumes too much for instance variable @ivar

Note that this example would trigger this smell warning as well:

class Parent
  def initialize(omg)
    @omg = omg
  end
end

class Child < Parent
  def foo
    @omg
  end
end

The way to address the smell warning is that you should create an attr_reader to use @omg in the subclass and not access @omg directly like this:

class Parent
  attr_reader :omg

  def initialize(omg)
    @omg = omg
  end
end

class Child < Parent
  def foo
    omg
  end
end

Directly accessing instance variables is considered a smell because it breaks encapsulation and makes it harder to reason about code.

If you don't want to expose those methods as public API just make them private like this:

class Parent
  def initialize(omg)
    @omg = omg
  end

  private
  attr_reader :omg
end

class Child < Parent
  def foo
    omg
  end
end

Current Support in Reek

An instance variable must:

  • be set in the constructor
  • or be accessed through a method with lazy initialization / memoization.

If not, Instance Variable Assumption will be reported.

Avoid too many return statements within this method.
Open

          return
Severity: Major
Found in app/controllers/users_controller.rb - About 30 mins to fix

    Avoid too many return statements within this method.
    Open

              return
    Severity: Major
    Found in app/controllers/users_controller.rb - About 30 mins to fix

      Avoid too many return statements within this method.
      Open

          return false if params[:user][:password].empty?
      Severity: Major
      Found in app/controllers/users_controller.rb - About 30 mins to fix

        Avoid too many return statements within this method.
        Open

                return
        Severity: Major
        Found in app/controllers/users_controller.rb - About 30 mins to fix

          Avoid too many return statements within this method.
          Open

                  return
          Severity: Major
          Found in app/controllers/users_controller.rb - About 30 mins to fix

            Avoid too many return statements within this method.
            Open

                      return
            Severity: Major
            Found in app/controllers/users_controller.rb - About 30 mins to fix

              Avoid too many return statements within this method.
              Open

                  return true
              Severity: Major
              Found in app/controllers/users_controller.rb - About 30 mins to fix

                Block has too many lines. [27/25]
                Open

                      format.html do
                        unless User.no_users_yet? || (@user && @user.is_admin?) || SITE_CONFIG['open_signups']
                          @page_title = t('users.no_signups_title')
                          @admin_email = SITE_CONFIG['admin_email']
                          render :action => "nosignup", :layout => "login"
                Severity: Minor
                Found in app/controllers/users_controller.rb by rubocop

                This cop checks if the length of a block exceeds some maximum value. Comment lines can optionally be ignored. The maximum allowed length is configurable. The cop can be configured to ignore blocks passed to certain methods.

                Complex method UsersController#check_create_user_params (20.4)
                Open

                  def check_create_user_params
                    return false unless params.key?(:user)
                    return false unless params[:user].key?(:login)
                    return false if params[:user][:login].empty?
                    return false unless params[:user].key?(:password)
                Severity: Minor
                Found in app/controllers/users_controller.rb by flog

                Flog calculates the ABC score for methods. The ABC score is based on assignments, branches (method calls), and conditions.

                You can read more about ABC metrics or the flog tool

                Complex method UsersController#update_password (20.2)
                Open

                  def update_password
                    # is used for focing password change after sha->bcrypt upgrade
                    current_user.change_password(user_params[:password], user_params[:password_confirmation])
                    notify :notice, t('users.password_updated')
                    redirect_to preferences_path
                Severity: Minor
                Found in app/controllers/users_controller.rb by flog

                Flog calculates the ABC score for methods. The ABC score is based on assignments, branches (method calls), and conditions.

                You can read more about ABC metrics or the flog tool

                Use safe navigation (&.) instead of checking if an object exists before calling the method.
                Open

                    elsif (@user && @user.is_admin?) || SITE_CONFIG['open_signups']
                Severity: Minor
                Found in app/controllers/users_controller.rb by rubocop

                This cop transforms usages of a method call safeguarded by a non nil check for the variable whose method is being called to safe navigation (&.).

                Configuration option: ConvertCodeThatCanStartToReturnNil The default for this is false. When configured to true, this will check for code in the format !foo.nil? && foo.bar. As it is written, the return of this code is limited to false and whatever the return of the method is. If this is converted to safe navigation, foo&.bar can start returning nil as well as what the method returns.

                Example:

                # bad
                foo.bar if foo
                foo.bar(param1, param2) if foo
                foo.bar { |e| e.something } if foo
                foo.bar(param) { |e| e.something } if foo
                
                foo.bar if !foo.nil?
                foo.bar unless !foo
                foo.bar unless foo.nil?
                
                foo && foo.bar
                foo && foo.bar(param1, param2)
                foo && foo.bar { |e| e.something }
                foo && foo.bar(param) { |e| e.something }
                
                # good
                foo&.bar
                foo&.bar(param1, param2)
                foo&.bar { |e| e.something }
                foo&.bar(param) { |e| e.something }
                
                foo.nil? || foo.bar
                !foo || foo.bar
                
                # Methods that `nil` will `respond_to?` should not be converted to
                # use safe navigation
                foo.to_i if foo

                Use safe navigation (&.) instead of checking if an object exists before calling the method.
                Open

                        signup_by_admin = true if @user && @user.is_admin?
                Severity: Minor
                Found in app/controllers/users_controller.rb by rubocop

                This cop transforms usages of a method call safeguarded by a non nil check for the variable whose method is being called to safe navigation (&.).

                Configuration option: ConvertCodeThatCanStartToReturnNil The default for this is false. When configured to true, this will check for code in the format !foo.nil? && foo.bar. As it is written, the return of this code is limited to false and whatever the return of the method is. If this is converted to safe navigation, foo&.bar can start returning nil as well as what the method returns.

                Example:

                # bad
                foo.bar if foo
                foo.bar(param1, param2) if foo
                foo.bar { |e| e.something } if foo
                foo.bar(param) { |e| e.something } if foo
                
                foo.bar if !foo.nil?
                foo.bar unless !foo
                foo.bar unless foo.nil?
                
                foo && foo.bar
                foo && foo.bar(param1, param2)
                foo && foo.bar { |e| e.something }
                foo && foo.bar(param) { |e| e.something }
                
                # good
                foo&.bar
                foo&.bar(param1, param2)
                foo&.bar { |e| e.something }
                foo&.bar(param) { |e| e.something }
                
                foo.nil? || foo.bar
                !foo || foo.bar
                
                # Methods that `nil` will `respond_to?` should not be converted to
                # use safe navigation
                foo.to_i if foo

                Redundant return detected.
                Open

                    return true
                Severity: Minor
                Found in app/controllers/users_controller.rb by rubocop

                This cop checks for redundant return expressions.

                Example:

                def test
                  return something
                end
                
                def test
                  one
                  two
                  three
                  return something
                end

                It should be extended to handle methods whose body is if/else or a case expression with a default branch.

                Use %i or %I for an array of symbols.
                Open

                  skip_before_action :login_required, :only => [:new, :create]
                Severity: Minor
                Found in app/controllers/users_controller.rb by rubocop

                This cop can check for array literals made up of symbols that are not using the %i() syntax.

                Alternatively, it checks for symbol arrays using the %i() syntax on projects which do not want to use that syntax.

                Configuration option: MinSize If set, arrays with fewer elements than this value will not trigger the cop. For example, a MinSize of3` will not enforce a style on an array of 2 or fewer elements.

                Example: EnforcedStyle: percent (default)

                # good
                %i[foo bar baz]
                
                # bad
                [:foo, :bar, :baz]

                Example: EnforcedStyle: brackets

                # good
                [:foo, :bar, :baz]
                
                # bad
                %i[foo bar baz]

                Do not use unless with else. Rewrite these with the positive case first.
                Open

                        unless user.new_record?
                          render :body => t('users.user_created'), :status => 200
                        else
                          render_failure user.errors.to_xml, 409
                        end
                Severity: Minor
                Found in app/controllers/users_controller.rb by rubocop

                This cop looks for unless expressions with else clauses.

                Example:

                # bad
                unless foo_bar.nil?
                  # do something...
                else
                  # do a different thing...
                end
                
                # good
                if foo_bar.present?
                  # do something...
                else
                  # do a different thing...
                end

                Line is too long. [121/120]
                Open

                      notify :warning, t('users.auth_type_update_error', :error_messages => current_user.errors.full_messages.join(', '))
                Severity: Minor
                Found in app/controllers/users_controller.rb by rubocop

                Do not use unless with else. Rewrite these with the positive case first.
                Open

                    unless session[:cas_user]
                      Tracks::Config.auth_schemes.each { |auth| @auth_types << [auth, auth] }
                    else
                      @auth_types << ['cas', 'cas']
                    end
                Severity: Minor
                Found in app/controllers/users_controller.rb by rubocop

                This cop looks for unless expressions with else clauses.

                Example:

                # bad
                unless foo_bar.nil?
                  # do something...
                else
                  # do a different thing...
                end
                
                # good
                if foo_bar.present?
                  # do something...
                else
                  # do a different thing...
                end

                Missing magic comment # frozen_string_literal: true.
                Open

                class UsersController < ApplicationController
                Severity: Minor
                Found in app/controllers/users_controller.rb by rubocop

                This cop is designed to help upgrade to Ruby 3.0. It will add the comment # frozen_string_literal: true to the top of files to enable frozen string literals. Frozen string literals may be default in Ruby 3.0. The comment will be added below a shebang and encoding comment. The frozen string literal comment is only valid in Ruby 2.3+.

                Example: EnforcedStyle: when_needed (default)

                # The `when_needed` style will add the frozen string literal comment
                # to files only when the `TargetRubyVersion` is set to 2.3+.
                # bad
                module Foo
                  # ...
                end
                
                # good
                # frozen_string_literal: true
                
                module Foo
                  # ...
                end

                Example: EnforcedStyle: always

                # The `always` style will always add the frozen string literal comment
                # to a file, regardless of the Ruby version or if `freeze` or `<<` are
                # called on a string literal.
                # bad
                module Bar
                  # ...
                end
                
                # good
                # frozen_string_literal: true
                
                module Bar
                  # ...
                end

                Example: EnforcedStyle: never

                # The `never` will enforce that the frozen string literal comment does
                # not exist in a file.
                # bad
                # frozen_string_literal: true
                
                module Baz
                  # ...
                end
                
                # good
                module Baz
                  # ...
                end

                Use %i or %I for an array of symbols.
                Open

                  prepend_before_action :login_optional, :only => [:new, :create]
                Severity: Minor
                Found in app/controllers/users_controller.rb by rubocop

                This cop can check for array literals made up of symbols that are not using the %i() syntax.

                Alternatively, it checks for symbol arrays using the %i() syntax on projects which do not want to use that syntax.

                Configuration option: MinSize If set, arrays with fewer elements than this value will not trigger the cop. For example, a MinSize of3` will not enforce a style on an array of 2 or fewer elements.

                Example: EnforcedStyle: percent (default)

                # good
                %i[foo bar baz]
                
                # bad
                [:foo, :bar, :baz]

                Example: EnforcedStyle: brackets

                # good
                [:foo, :bar, :baz]
                
                # bad
                %i[foo bar baz]

                Avoid rescuing the Exception class. Perhaps you meant to rescue StandardError?
                Open

                  rescue Exception => error
                    notify :error, error.message
                    redirect_to change_password_user_path(current_user)
                Severity: Minor
                Found in app/controllers/users_controller.rb by rubocop

                This cop checks for rescue blocks targeting the Exception class.

                Example:

                # bad
                
                begin
                  do_something
                rescue Exception
                  handle_exception
                end

                Example:

                # good
                
                begin
                  do_something
                rescue ArgumentError
                  handle_exception
                end

                Line is too long. [134/120]
                Open

                    params.require(:user).permit(:login, :first_name, :last_name, :email, :password_confirmation, :password, :auth_type, :open_id_url)
                Severity: Minor
                Found in app/controllers/users_controller.rb by rubocop

                Line is too long. [139/120]
                Open

                          render_failure "Expected post format is valid xml like so: <user><login>username</login><password>abc123</password></user>.", 400
                Severity: Minor
                Found in app/controllers/users_controller.rb by rubocop

                Use safe navigation (&.) instead of checking if an object exists before calling the method.
                Open

                        unless User.no_users_yet? || (@user && @user.is_admin?) || SITE_CONFIG['open_signups']
                Severity: Minor
                Found in app/controllers/users_controller.rb by rubocop

                This cop transforms usages of a method call safeguarded by a non nil check for the variable whose method is being called to safe navigation (&.).

                Configuration option: ConvertCodeThatCanStartToReturnNil The default for this is false. When configured to true, this will check for code in the format !foo.nil? && foo.bar. As it is written, the return of this code is limited to false and whatever the return of the method is. If this is converted to safe navigation, foo&.bar can start returning nil as well as what the method returns.

                Example:

                # bad
                foo.bar if foo
                foo.bar(param1, param2) if foo
                foo.bar { |e| e.something } if foo
                foo.bar(param) { |e| e.something } if foo
                
                foo.bar if !foo.nil?
                foo.bar unless !foo
                foo.bar unless foo.nil?
                
                foo && foo.bar
                foo && foo.bar(param1, param2)
                foo && foo.bar { |e| e.something }
                foo && foo.bar(param) { |e| e.something }
                
                # good
                foo&.bar
                foo&.bar(param1, param2)
                foo&.bar { |e| e.something }
                foo&.bar(param) { |e| e.something }
                
                foo.nil? || foo.bar
                !foo || foo.bar
                
                # Methods that `nil` will `respond_to?` should not be converted to
                # use safe navigation
                foo.to_i if foo

                Favor modifier if usage when having a single-line body. Another good alternative is the usage of control flow &&/||.
                Open

                      if params[:order] && User.column_names.include?(params[:order])
                Severity: Minor
                Found in app/controllers/users_controller.rb by rubocop

                Checks for if and unless statements that would fit on one line if written as a modifier if/unless. The maximum line length is configured in the Metrics/LineLength cop.

                Example:

                # bad
                if condition
                  do_stuff(bar)
                end
                
                unless qux.empty?
                  Foo.do_something
                end
                
                # good
                do_stuff(bar) if condition
                Foo.do_something unless qux.empty?

                Use safe navigation (&.) instead of checking if an object exists before calling the method.
                Open

                        unless current_user && current_user.is_admin
                Severity: Minor
                Found in app/controllers/users_controller.rb by rubocop

                This cop transforms usages of a method call safeguarded by a non nil check for the variable whose method is being called to safe navigation (&.).

                Configuration option: ConvertCodeThatCanStartToReturnNil The default for this is false. When configured to true, this will check for code in the format !foo.nil? && foo.bar. As it is written, the return of this code is limited to false and whatever the return of the method is. If this is converted to safe navigation, foo&.bar can start returning nil as well as what the method returns.

                Example:

                # bad
                foo.bar if foo
                foo.bar(param1, param2) if foo
                foo.bar { |e| e.something } if foo
                foo.bar(param) { |e| e.something } if foo
                
                foo.bar if !foo.nil?
                foo.bar unless !foo
                foo.bar unless foo.nil?
                
                foo && foo.bar
                foo && foo.bar(param1, param2)
                foo && foo.bar { |e| e.something }
                foo && foo.bar(param) { |e| e.something }
                
                # good
                foo&.bar
                foo&.bar(param1, param2)
                foo&.bar { |e| e.something }
                foo&.bar(param) { |e| e.something }
                
                foo.nil? || foo.bar
                !foo || foo.bar
                
                # Methods that `nil` will `respond_to?` should not be converted to
                # use safe navigation
                foo.to_i if foo

                Useless assignment to variable - saved.
                Open

                        saved = user.save
                Severity: Minor
                Found in app/controllers/users_controller.rb by rubocop

                This cop checks for every useless assignment to local variable in every scope. The basic idea for this cop was from the warning of ruby -cw:

                assigned but unused variable - foo

                Currently this cop has advanced logic that detects unreferenced reassignments and properly handles varied cases such as branch, loop, rescue, ensure, etc.

                Example:

                # bad
                
                def some_method
                  some_var = 1
                  do_something
                end

                Example:

                # good
                
                def some_method
                  some_var = 1
                  do_something(some_var)
                end

                Use %w or %W for an array of words.
                Open

                      @auth_types << ['cas', 'cas']
                Severity: Minor
                Found in app/controllers/users_controller.rb by rubocop

                This cop can check for array literals made up of word-like strings, that are not using the %w() syntax.

                Alternatively, it can check for uses of the %w() syntax, in projects which do not want to include that syntax.

                Configuration option: MinSize If set, arrays with fewer elements than this value will not trigger the cop. For example, a MinSize of 3 will not enforce a style on an array of 2 or fewer elements.

                Example: EnforcedStyle: percent (default)

                # good
                %w[foo bar baz]
                
                # bad
                ['foo', 'bar', 'baz']

                Example: EnforcedStyle: brackets

                # good
                ['foo', 'bar', 'baz']
                
                # bad
                %w[foo bar baz]

                Do not prefix reader method names with get_.
                Open

                  def get_new_user
                Severity: Minor
                Found in app/controllers/users_controller.rb by rubocop

                This cop makes sure that accessor methods are named properly.

                Example:

                # bad
                def set_attribute(value)
                end
                
                # good
                def attribute=(value)
                end
                
                # bad
                def get_attribute
                end
                
                # good
                def attribute
                end

                Line is too long. [130/120]
                Open

                      render_failure "Expected post format is valid xml like so: <user><login>username</login><password>abc123</password></user>."
                Severity: Minor
                Found in app/controllers/users_controller.rb by rubocop

                Favor modifier if usage when having a single-line body. Another good alternative is the usage of control flow &&/||.
                Open

                    if @saved && current_user == @deleted_user
                Severity: Minor
                Found in app/controllers/users_controller.rb by rubocop

                Checks for if and unless statements that would fit on one line if written as a modifier if/unless. The maximum line length is configured in the Metrics/LineLength cop.

                Example:

                # bad
                if condition
                  do_stuff(bar)
                end
                
                unless qux.empty?
                  Foo.do_something
                end
                
                # good
                do_stuff(bar) if condition
                Foo.do_something unless qux.empty?

                Use %i or %I for an array of symbols.
                Open

                  before_action :admin_login_required, :only => [:index, :show]
                Severity: Minor
                Found in app/controllers/users_controller.rb by rubocop

                This cop can check for array literals made up of symbols that are not using the %i() syntax.

                Alternatively, it checks for symbol arrays using the %i() syntax on projects which do not want to use that syntax.

                Configuration option: MinSize If set, arrays with fewer elements than this value will not trigger the cop. For example, a MinSize of3` will not enforce a style on an array of 2 or fewer elements.

                Example: EnforcedStyle: percent (default)

                # good
                %i[foo bar baz]
                
                # bad
                [:foo, :bar, :baz]

                Example: EnforcedStyle: brackets

                # good
                [:foo, :bar, :baz]
                
                # bad
                %i[foo bar baz]

                There are no issues that match your filters.

                Category
                Status