Fields::FormBuilder#user_defined_form has boolean parameter 'show_readonly_inputs' Open
def user_defined_form(show_readonly_inputs: true)
- Read upRead up
- Exclude checks
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
Fields::FormBuilder#user_defined_form is controlled by argument 'show_readonly_inputs' Open
field.editable_by?(user) && (show_readonly_inputs || !field.read_only)
- Read upRead up
- Exclude checks
Control Parameter
is a special case of Control Couple
Example
A simple example would be the "quoted" parameter in the following method:
def write(quoted)
if quoted
write_quoted @value
else
write_unquoted @value
end
end
Fixing those problems is out of the scope of this document but an easy solution could be to remove the "write" method alltogether and to move the calls to "writequoted" / "writeunquoted" in the initial caller of "write".
Fields::FormBuilder assumes too much for instance variable '@object' Open
class Fields::FormBuilder < ThreeScale::SemanticFormBuilder
- 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.
Fields::FormBuilder assumes too much for instance variable '@template' Open
class Fields::FormBuilder < ThreeScale::SemanticFormBuilder
- 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.
Fields::FormBuilder#field performs a nil-check Open
output_html(field, options).html_safe unless field.nil?
- Read upRead up
- Exclude checks
A NilCheck
is a type check. Failures of NilCheck
violate the "tell, don't ask" principle.
Additionally, type checks often mask bigger problems in your source code like not using OOP and / or polymorphism when you should.
Example
Given
class Klass
def nil_checker(argument)
if argument.nil?
puts "argument isn't nil!"
end
end
end
Reek would emit the following warning:
test.rb -- 1 warning:
[3]:Klass#nil_checker performs a nil-check. (NilCheck)