andypike/rectify

View on GitHub

Showing 87 of 91 total issues

Rectify::RSpec::DatabaseReporter::QueryStats#each yields 4 parameters
Open

            yield(

A Long Yield List occurs when a method yields a lot of arguments to the block it gets passed.

Example

class Dummy
  def yields_a_lot(foo,bar,baz,fling,flung)
    yield foo,bar,baz,fling,flung
  end
end

Reek would report the following warning:

test.rb -- 1 warning:
  [4]:Dummy#yields_a_lot yields 5 parameters (LongYieldList)

A common solution to this problem would be the introduction of parameter objects.

Potential XSS vulnerability in Action View
Open

    actionview (5.2.1)
Severity: Minor
Found in Gemfile.lock by bundler-audit

Advisory: CVE-2020-15169

URL: https://groups.google.com/g/rubyonrails-security/c/b-C9kSGXYrc

Solution: upgrade to >= 5.2.4.4, ~> 5.2.4, >= 6.0.3.3

Loofah XSS Vulnerability
Open

    loofah (2.2.2)
Severity: Minor
Found in Gemfile.lock by bundler-audit

Advisory: CVE-2018-16468

Criticality: Medium

URL: https://github.com/flavorjones/loofah/issues/154

Solution: upgrade to >= 2.2.3

Rectify::Presenter#respond_to_missing? has boolean parameter 'include_private'
Open

    def respond_to_missing?(method_name, include_private = false)
Severity: Minor
Found in lib/rectify/presenter.rb by reek

Boolean Parameter is a special case of Control Couple, where a method parameter is defaulted to true or false. A Boolean Parameter effectively permits a method's caller to decide which execution path to take. This is a case of bad cohesion. You're creating a dependency between methods that is not really necessary, thus increasing coupling.

Example

Given

class Dummy
  def hit_the_switch(switch = true)
    if switch
      puts 'Hitting the switch'
      # do other things...
    else
      puts 'Not hitting the switch'
      # do other things...
    end
  end
end

Reek would emit the following warning:

test.rb -- 3 warnings:
  [1]:Dummy#hit_the_switch has boolean parameter 'switch' (BooleanParameter)
  [2]:Dummy#hit_the_switch is controlled by argument switch (ControlParameter)

Note that both smells are reported, Boolean Parameter and Control Parameter.

Getting rid of the smell

This is highly dependent on your exact architecture, but looking at the example above what you could do is:

  • Move everything in the if branch into a separate method
  • Move everything in the else branch into a separate method
  • Get rid of the hit_the_switch method alltogether
  • Make the decision what method to call in the initial caller of hit_the_switch

Rectify::Command#respond_to_missing? has boolean parameter 'include_private'
Open

    def respond_to_missing?(method_name, include_private = false)
Severity: Minor
Found in lib/rectify/command.rb by reek

Boolean Parameter is a special case of Control Couple, where a method parameter is defaulted to true or false. A Boolean Parameter effectively permits a method's caller to decide which execution path to take. This is a case of bad cohesion. You're creating a dependency between methods that is not really necessary, thus increasing coupling.

Example

Given

class Dummy
  def hit_the_switch(switch = true)
    if switch
      puts 'Hitting the switch'
      # do other things...
    else
      puts 'Not hitting the switch'
      # do other things...
    end
  end
end

Reek would emit the following warning:

test.rb -- 3 warnings:
  [1]:Dummy#hit_the_switch has boolean parameter 'switch' (BooleanParameter)
  [2]:Dummy#hit_the_switch is controlled by argument switch (ControlParameter)

Note that both smells are reported, Boolean Parameter and Control Parameter.

Getting rid of the smell

This is highly dependent on your exact architecture, but looking at the example above what you could do is:

  • Move everything in the if branch into a separate method
  • Move everything in the else branch into a separate method
  • Get rid of the hit_the_switch method alltogether
  • Make the decision what method to call in the initial caller of hit_the_switch

Rectify::RSpec::DatabaseReporter::QueryStats#each refers to 'infos' more than self (maybe move it to another class?)
Open

              infos.first.type,
              infos.count,
              infos.sum(&:time).round(5)

Feature Envy occurs when a code fragment references another object more often than it references itself, or when several clients do the same series of manipulations on a particular type of object.

Feature Envy reduces the code's ability to communicate intent: code that "belongs" on one class but which is located in another can be hard to find, and may upset the "System of Names" in the host class.

Feature Envy also affects the design's flexibility: A code fragment that is in the wrong class creates couplings that may not be natural within the application's domain, and creates a loss of cohesion in the unwilling host class.

Feature Envy often arises because it must manipulate other objects (usually its arguments) to get them into a useful form, and one force preventing them (the arguments) doing this themselves is that the common knowledge lives outside the arguments, or the arguments are of too basic a type to justify extending that type. Therefore there must be something which 'knows' about the contents or purposes of the arguments. That thing would have to be more than just a basic type, because the basic types are either containers which don't know about their contents, or they are single objects which can't capture their relationship with their fellows of the same type. So, this thing with the extra knowledge should be reified into a class, and the utility method will most likely belong there.

Example

Running Reek on:

class Warehouse
  def sale_price(item)
    (item.price - item.rebate) * @vat
  end
end

would report:

Warehouse#total_price refers to item more than self (FeatureEnvy)

since this:

(item.price - item.rebate)

belongs to the Item class, not the Warehouse.

Possible Strong Parameters Bypass in ActionPack
Open

    actionpack (5.2.1)
Severity: Minor
Found in Gemfile.lock by bundler-audit

Advisory: CVE-2020-8164

URL: https://groups.google.com/forum/#!topic/rubyonrails-security/f6ioe4sdpbY

Solution: upgrade to >= 5.2.4.3, ~> 5.2.4, >= 6.0.3.1

Possible DoS vulnerability in Rack
Open

    rack (2.0.5)
Severity: Minor
Found in Gemfile.lock by bundler-audit

Advisory: CVE-2018-16470

URL: https://groups.google.com/forum/#!topic/ruby-security-ann/Dz4sRl-ktKk

Solution: upgrade to >= 2.0.6

Possible information leak / session hijack vulnerability
Open

    rack (2.0.5)
Severity: Minor
Found in Gemfile.lock by bundler-audit

Advisory: CVE-2019-16782

URL: https://github.com/rack/rack/security/advisories/GHSA-hrqr-hxpp-chr3

Solution: upgrade to ~> 1.6.12, >= 2.0.8

Rectify::RSpec::DatabaseReporter::QueryStats#add has 4 parameters
Open

        def add(example, start, finish, query)

A Long Parameter List occurs when a method has a lot of parameters.

Example

Given

class Dummy
  def long_list(foo,bar,baz,fling,flung)
    puts foo,bar,baz,fling,flung
  end
end

Reek would report the following warning:

test.rb -- 1 warning:
  [2]:Dummy#long_list has 5 parameters (LongParameterList)

A common solution to this problem would be the introduction of parameter objects.

Rectify::StubForm#respond_to_missing? has boolean parameter '_include_private'
Open

    def respond_to_missing?(method_name, _include_private = false)
Severity: Minor
Found in lib/rectify/rspec/stub_form.rb by reek

Boolean Parameter is a special case of Control Couple, where a method parameter is defaulted to true or false. A Boolean Parameter effectively permits a method's caller to decide which execution path to take. This is a case of bad cohesion. You're creating a dependency between methods that is not really necessary, thus increasing coupling.

Example

Given

class Dummy
  def hit_the_switch(switch = true)
    if switch
      puts 'Hitting the switch'
      # do other things...
    else
      puts 'Not hitting the switch'
      # do other things...
    end
  end
end

Reek would emit the following warning:

test.rb -- 3 warnings:
  [1]:Dummy#hit_the_switch has boolean parameter 'switch' (BooleanParameter)
  [2]:Dummy#hit_the_switch is controlled by argument switch (ControlParameter)

Note that both smells are reported, Boolean Parameter and Control Parameter.

Getting rid of the smell

This is highly dependent on your exact architecture, but looking at the example above what you could do is:

  • Move everything in the if branch into a separate method
  • Move everything in the else branch into a separate method
  • Get rid of the hit_the_switch method alltogether
  • Make the decision what method to call in the initial caller of hit_the_switch

Rectify::RSpec::DatabaseReporter::QueryStats#add refers to 'info' more than self (maybe move it to another class?)
Open

          return if info.ignore?

          stats[info.target] << info

Feature Envy occurs when a code fragment references another object more often than it references itself, or when several clients do the same series of manipulations on a particular type of object.

Feature Envy reduces the code's ability to communicate intent: code that "belongs" on one class but which is located in another can be hard to find, and may upset the "System of Names" in the host class.

Feature Envy also affects the design's flexibility: A code fragment that is in the wrong class creates couplings that may not be natural within the application's domain, and creates a loss of cohesion in the unwilling host class.

Feature Envy often arises because it must manipulate other objects (usually its arguments) to get them into a useful form, and one force preventing them (the arguments) doing this themselves is that the common knowledge lives outside the arguments, or the arguments are of too basic a type to justify extending that type. Therefore there must be something which 'knows' about the contents or purposes of the arguments. That thing would have to be more than just a basic type, because the basic types are either containers which don't know about their contents, or they are single objects which can't capture their relationship with their fellows of the same type. So, this thing with the extra knowledge should be reified into a class, and the utility method will most likely belong there.

Example

Running Reek on:

class Warehouse
  def sale_price(item)
    (item.price - item.rebate) * @vat
  end
end

would report:

Warehouse#total_price refers to item more than self (FeatureEnvy)

since this:

(item.price - item.rebate)

belongs to the Item class, not the Warehouse.

Rectify::FormAttribute#collection_of_form_objects? manually dispatches method call
Open

      collection? && element_class.respond_to?(:from_model)
Severity: Minor
Found in lib/rectify/form_attribute.rb by reek

Reek reports a Manual Dispatch smell if it finds source code that manually checks whether an object responds to a method before that method is called. Manual dispatch is a type of Simulated Polymorphism which leads to code that is harder to reason about, debug, and refactor.

Example

class MyManualDispatcher
  attr_reader :foo

  def initialize(foo)
    @foo = foo
  end

  def call
    foo.bar if foo.respond_to?(:bar)
  end
end

Reek would emit the following warning:

test.rb -- 1 warning:
  [9]: MyManualDispatcher manually dispatches method call (ManualDispatch)

Rectify::FormatAttributesHash#convert_hash_keys calls 'convert_hash_keys(v)' 2 times
Open

        value.map { |v| convert_hash_keys(v) }
      when Hash
        Hash[value.map { |k, v| [underscore_key(k), convert_hash_keys(v)] }]
Severity: Minor
Found in lib/rectify/format_attributes_hash.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.

Rectify::RSpec::StubQuery has no descriptive comment
Open

    class StubQuery < Query
Severity: Minor
Found in lib/rectify/rspec/stub_query.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

Rectify::Command#method_missing manually dispatches method call
Open

      if @caller.respond_to?(method_name, true)
Severity: Minor
Found in lib/rectify/command.rb by reek

Reek reports a Manual Dispatch smell if it finds source code that manually checks whether an object responds to a method before that method is called. Manual dispatch is a type of Simulated Polymorphism which leads to code that is harder to reason about, debug, and refactor.

Example

class MyManualDispatcher
  attr_reader :foo

  def initialize(foo)
    @foo = foo
  end

  def call
    foo.bar if foo.respond_to?(:bar)
  end
end

Reek would emit the following warning:

test.rb -- 1 warning:
  [9]: MyManualDispatcher manually dispatches method call (ManualDispatch)

Rectify::FormatAttributesHash#convert_hash_keys calls 'value.map' 2 times
Open

        value.map { |v| convert_hash_keys(v) }
      when Hash
        Hash[value.map { |k, v| [underscore_key(k), convert_hash_keys(v)] }]
Severity: Minor
Found in lib/rectify/format_attributes_hash.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.

Rectify::Command#respond_to_missing? manually dispatches method call
Open

      @caller.respond_to?(method_name, include_private)
Severity: Minor
Found in lib/rectify/command.rb by reek

Reek reports a Manual Dispatch smell if it finds source code that manually checks whether an object responds to a method before that method is called. Manual dispatch is a type of Simulated Polymorphism which leads to code that is harder to reason about, debug, and refactor.

Example

class MyManualDispatcher
  attr_reader :foo

  def initialize(foo)
    @foo = foo
  end

  def call
    foo.bar if foo.respond_to?(:bar)
  end
end

Reek would emit the following warning:

test.rb -- 1 warning:
  [9]: MyManualDispatcher manually dispatches method call (ManualDispatch)

Rectify::FormatAttributesHash has no descriptive comment
Open

  class FormatAttributesHash
Severity: Minor
Found in lib/rectify/format_attributes_hash.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

Rectify::Presenter#respond_to_missing? manually dispatches method call
Open

      view_context.respond_to?(method_name, include_private)
Severity: Minor
Found in lib/rectify/presenter.rb by reek

Reek reports a Manual Dispatch smell if it finds source code that manually checks whether an object responds to a method before that method is called. Manual dispatch is a type of Simulated Polymorphism which leads to code that is harder to reason about, debug, and refactor.

Example

class MyManualDispatcher
  attr_reader :foo

  def initialize(foo)
    @foo = foo
  end

  def call
    foo.bar if foo.respond_to?(:bar)
  end
end

Reek would emit the following warning:

test.rb -- 1 warning:
  [9]: MyManualDispatcher manually dispatches method call (ManualDispatch)
Severity
Category
Status
Source
Language