diff --git a/spec/ameba/rules/negated_conditions_in_unless_spec.cr b/spec/ameba/rules/negated_conditions_in_unless_spec.cr new file mode 100644 index 00000000..5c29ae49 --- /dev/null +++ b/spec/ameba/rules/negated_conditions_in_unless_spec.cr @@ -0,0 +1,68 @@ +require "../../spec_helper" + +module Ameba::Rules + subject = NegatedConditionsInUnless.new + + describe NegatedConditionsInUnless do + it "passes with a unless without negated condition" do + s = Source.new %( + unless a + :ok + end + + :ok unless b + + unless s.empty? + :ok + end + ) + subject.catch(s).valid?.should be_true + end + + it "fails if there is a negated condition in unless" do + s = Source.new %( + unless !a + :nok + end + ) + subject.catch(s).valid?.should be_false + end + + it "fails if one of AND conditions is negated" do + s = Source.new %( + unless a && !b + :nok + end + ) + subject.catch(s).valid?.should be_false + end + + it "fails if one of OR conditions is negated" do + s = Source.new %( + unless a || !b + :nok + end + ) + subject.catch(s).valid?.should be_false + end + + it "fails if one of inner conditions is negated" do + s = Source.new %( + unless a && (b || !c) + :nok + end + ) + subject.catch(s).valid?.should be_false + end + + it "reports rule, pos and message" do + s = Source.new ":nok unless !s.empty?" + subject.catch(s).valid?.should be_false + + error = s.errors.first + error.rule.should_not be_nil + error.pos.should eq 1 + error.message.should eq "Avoid negated conditions in unless blocks" + end + end +end diff --git a/src/ameba/rules/negated_conditions_in_unless.cr b/src/ameba/rules/negated_conditions_in_unless.cr new file mode 100644 index 00000000..b7ba7ca7 --- /dev/null +++ b/src/ameba/rules/negated_conditions_in_unless.cr @@ -0,0 +1,47 @@ +module Ameba::Rules + # A rule that disallows negated conditions in unless. + # + # For example, this is considered invalid: + # + # ```crystal + # unless !s.empty? + # :ok + # end + # ``` + # + # And should be rewritten to the following: + # + # ```crystal + # if s.emtpy? + # :ok + # end + # ``` + # + # It is pretty difficult to wrap your head around a block of code + # that is executed if a negated condition is NOT met. + # + struct NegatedConditionsInUnless < Rule + def test(source) + AST::UnlessVisitor.new self, source + end + + def test(source, node : Crystal::Unless) + return unless negated_condition? node.cond + source.error self, node.location.try &.line_number, + "Avoid negated conditions in unless blocks" + end + + private def negated_condition?(node) + case node + when Crystal::BinaryOp + negated_condition?(node.left) || negated_condition?(node.right) + when Crystal::Expressions + node.expressions.any? { |e| negated_condition? e } + when Crystal::Not + true + else + false + end + end + end +end