From 67506fc6430053872486ec94273bc393c47b04f5 Mon Sep 17 00:00:00 2001
From: Vitalii Elenhaupt <velenhaupt@gmail.com>
Date: Wed, 1 Nov 2017 21:29:37 +0200
Subject: [PATCH] New rule: negated conditions in unless

---
 .../negated_conditions_in_unless_spec.cr      | 68 +++++++++++++++++++
 .../rules/negated_conditions_in_unless.cr     | 47 +++++++++++++
 2 files changed, 115 insertions(+)
 create mode 100644 spec/ameba/rules/negated_conditions_in_unless_spec.cr
 create mode 100644 src/ameba/rules/negated_conditions_in_unless.cr

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