Class User
has 25 methods (exceeds 20 allowed). Consider refactoring. Open
class User < Drops::Model
allowed_name :user, :users
drop_example %{
<h1>User {{ user.display_name }}</h1>
Liquid::Drops::User has at least 25 methods Open
class User < Drops::Model
- Read upRead up
- Exclude checks
Too Many Methods
is a special case of LargeClass
.
Example
Given this configuration
TooManyMethods:
max_methods: 3
and this code:
class TooManyMethods
def one; end
def two; end
def three; end
def four; end
end
Reek would emit the following warning:
test.rb -- 1 warning:
[1]:TooManyMethods has at least 4 methods (TooManyMethods)
Liquid::Drops::User#roles_collection calls '::User.current' 2 times Open
if role == :member && ::Ability.new(::User.current).can?(:create_contributors, ::User.current.account)
- Read upRead up
- Exclude checks
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.
Liquid::Drops::User assumes too much for instance variable '@model' Open
class User < Drops::Model
- Read upRead up
- Exclude checks
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.
Liquid::Drops::User#roles_collection doesn't depend on instance state (maybe move it to another class?) Open
def roles_collection
- Read upRead up
- Exclude checks
A Utility Function is any instance method that has no dependency on the state of the instance.