Optimize rules (#185)

* Replace pointless interpolations with actual values

* Rules optimizations

* Stylistic refactors

* Remove extraneous blank lines

* Remove some instances of .not_nil! usage

spec/ameba/rule/lint/redundant_with_object_spec.cr

@@ -77,7 +77,7 @@ module Ameba::Rule::Lint
       issue.rule.should_not be_nil
       issue.location.to_s.should eq "source.cr:2:14"
       issue.end_location.to_s.should eq "source.cr:2:30"
-      issue.message.should eq "Use each instead of each_with_object"
+      issue.message.should eq "Use `each` instead of `each_with_object`"
     end
   end
 end

src/ameba/cli/cmd.cr

@@ -8,8 +8,13 @@ module Ameba::Cli
   def run(args = ARGV)
     opts = parse_args args
     config = Config.load opts.config, opts.colors?
-    config.globs = opts.globs.not_nil! if opts.globs
+
-    config.severity = opts.fail_level.not_nil! if opts.fail_level
+    if globs = opts.globs
+      config.globs = globs
+    end
+    if fail_level = opts.fail_level
+      config.severity = fail_level
+    end
 
     configure_formatter(config, opts)
     configure_rules(config, opts)

src/ameba/rule/layout/line_length.cr

@@ -8,7 +8,6 @@ module Ameba::Rule::Layout
   #   Enabled: true
   #   MaxLength: 100
   # ```
-  #
   struct LineLength < Base
     properties do
       enabled false
@@ -20,9 +19,7 @@ module Ameba::Rule::Layout
 
     def test(source)
       source.lines.each_with_index do |line, index|
-        next unless line.size > max_length
+        issue_for({index + 1, max_length + 1}, MSG) if line.size > max_length
-
-        issue_for({index + 1, max_length + 1}, MSG)
       end
     end
   end

src/ameba/rule/layout/trailing_blank_lines.cr

@@ -7,7 +7,6 @@ module Ameba::Rule::Layout
   # Layout/TrailingBlankLines:
   #   Enabled: true
   # ```
-  #
   struct TrailingBlankLines < Base
     properties do
       description "Disallows trailing blank lines"

src/ameba/rule/layout/trailing_whitespace.cr

@@ -7,7 +7,6 @@ module Ameba::Rule::Layout
   # Layout/TrailingWhitespace:
   #   Enabled: true
   # ```
-  #
   struct TrailingWhitespace < Base
     properties do
       description "Disallows trailing whitespaces"
@@ -17,8 +16,7 @@ module Ameba::Rule::Layout
 
     def test(source)
       source.lines.each_with_index do |line, index|
-        next unless line =~ /\s$/
+        issue_for({index + 1, line.size}, MSG) if line =~ /\s$/
-        issue_for({index + 1, line.size}, MSG)
       end
     end
   end

src/ameba/rule/lint/bad_directive.cr

@@ -17,7 +17,6 @@ module Ameba::Rule::Lint
   # Lint/BadDirective:
   #   Enabled: true
   # ```
-  #
   struct BadDirective < Base
     properties do
       description "Reports bad comment directives"
@@ -48,7 +47,9 @@ module Ameba::Rule::Lint
 
     private def check_rules(source, token, rules)
       bad_names = rules - ALL_RULE_NAMES - ALL_GROUP_NAMES
-      issue_for token, "Such rules do not exist: %s" % bad_names.join(", ") unless bad_names.empty?
+      return if bad_names.empty?
+
+      issue_for token, "Such rules do not exist: %s" % bad_names.join(", ")
     end
   end
 end

src/ameba/rule/lint/comparison_to_boolean.cr

@@ -19,7 +19,6 @@ module Ameba::Rule::Lint
   # Lint/ComparisonToBoolean:
   #   Enabled: true
   # ```
-  #
   struct ComparisonToBoolean < Base
     properties do
       enabled false
@@ -33,9 +32,7 @@ module Ameba::Rule::Lint
       to_boolean = node.args.first?.try &.is_a?(Crystal::BoolLiteral) ||
                    node.obj.is_a?(Crystal::BoolLiteral)
 
-      return unless comparison && to_boolean
+      issue_for node, MSG if comparison && to_boolean
-
-      issue_for node, MSG
     end
   end
 end

src/ameba/rule/lint/debugger_statement.cr

@@ -10,7 +10,6 @@ module Ameba::Rule::Lint
   # Lint/DebuggerStatement:
   #   Enabled: true
   # ```
-  #
   struct DebuggerStatement < Base
     properties do
       description "Disallows calls to debugger"

src/ameba/rule/lint/empty_ensure.cr

@@ -38,7 +38,6 @@ module Ameba::Rule::Lint
   # Lint/EmptyEnsure
   #   Enabled: true
   # ```
-  #
   struct EmptyEnsure < Base
     properties do
       description "Disallows empty ensure statement"

src/ameba/rule/lint/empty_expression.cr

@@ -27,7 +27,6 @@ module Ameba::Rule::Lint
   # Lint/EmptyExpression:
   #   Enabled: true
   # ```
-  #
   struct EmptyExpression < Base
     include AST::Util
 
@@ -41,8 +40,7 @@ module Ameba::Rule::Lint
 
     def test(source, node : Crystal::NilLiteral)
       exp = node_source(node, source.lines).try &.join
-
+      return if exp.in?(nil, "nil")
-      return if exp.nil? || exp == "nil"
 
       issue_for node, MSG % exp
     end

src/ameba/rule/lint/hash_duplicated_key.cr

@@ -19,7 +19,6 @@ module Ameba::Rule::Lint
   # Lint/HashDuplicatedKey:
   #   Enabled: true
   # ```
-  #
   struct HashDuplicatedKey < Base
     properties do
       description "Disallows duplicated keys in hash literals"
@@ -28,7 +27,7 @@ module Ameba::Rule::Lint
     MSG = "Duplicated keys in hash literal: %s"
 
     def test(source, node : Crystal::HashLiteral)
-      return unless (keys = duplicated_keys(node.entries)).any?
+      return if (keys = duplicated_keys(node.entries)).empty?
 
       issue_for node, MSG % keys.join(", ")
     end
@@ -36,7 +35,7 @@ module Ameba::Rule::Lint
     private def duplicated_keys(entries)
       entries.map(&.key)
         .group_by(&.itself)
-        .select { |_, v| v.size > 1 }
+        .tap(&.select! { |_, v| v.size > 1 })
         .map { |k, _| k }
     end
   end

src/ameba/rule/lint/literal_in_condition.cr

@@ -19,7 +19,6 @@ module Ameba::Rule::Lint
   # Lint/LiteralInCondition:
   #   Enabled: true
   # ```
-  #
   struct LiteralInCondition < Base
     include AST::Util
 

src/ameba/rule/lint/literal_in_interpolation.cr

@@ -15,7 +15,6 @@ module Ameba::Rule::Lint
   # Lint/LiteralInInterpolation
   #   Enabled: true
   # ```
-  #
   struct LiteralInInterpolation < Base
     include AST::Util
 

src/ameba/rule/lint/percent_array.cr

@@ -23,7 +23,6 @@ module Ameba::Rule::Lint
   #   StringArrayUnwantedSymbols: ',"'
   #   SymbolArrayUnwantedSymbols: ',:'
   # ```
-  #
   struct PercentArrays < Base
     properties do
       description "Disallows some unwanted symbols in percent array literals"

src/ameba/rule/lint/rand_zero.cr

@@ -22,7 +22,6 @@ module Ameba::Rule::Lint
   # Lint/RandZero:
   #   Enabled: true
   # ```
-  #
   struct RandZero < Base
     properties do
       description "Disallows rand zero calls"
@@ -36,7 +35,7 @@ module Ameba::Rule::Lint
                     (arg = node.args.first) &&
                     (arg.is_a? Crystal::NumberLiteral) &&
                     (value = arg.value) &&
-                    (value == "0" || value == "1")
+                    value.in?("0", "1")
 
       issue_for node, MSG % node
     end

src/ameba/rule/lint/redundant_string_coercion.cr

@@ -20,7 +20,6 @@ module Ameba::Rule::Lint
   # Lint/RedundantStringCoersion
   #   Enabled: true
   # ```
-  #
   struct RedundantStringCoercion < Base
     include AST::Util
 

src/ameba/rule/lint/redundant_with_index.cr

@@ -26,7 +26,6 @@ module Ameba::Rule::Lint
   # Lint/RedundantWithIndex:
   #   Enabled: true
   # ```
-  #
   struct RedundantWithIndex < Base
     properties do
       description "Disallows redundant `with_index` calls"
@@ -35,7 +34,8 @@ module Ameba::Rule::Lint
     def test(source, node : Crystal::Call)
       args, block = node.args, node.block
 
-      return if args.size > 1 || block.nil? || with_index_arg?(block.not_nil!)
+      return if block.nil? || args.size > 1
+      return if with_index_arg?(block)
 
       case node.name
       when "with_index"

src/ameba/rule/lint/redundant_with_object.cr

@@ -27,21 +27,20 @@ module Ameba::Rule::Lint
   # Lint/RedundantWithObject:
   #   Enabled: true
   # ```
-  #
   struct RedundantWithObject < Base
     properties do
       description "Disallows redundant `with_object` calls"
     end
 
+    MSG = "Use `each` instead of `each_with_object`"
+
     def test(source, node : Crystal::Call)
       return if node.name != "each_with_object" ||
                 node.args.size != 1 ||
                 node.block.nil? ||
                 with_index_arg?(node.block.not_nil!)
 
-      issue_for node.name_location,
+      issue_for node.name_location, node.name_end_location, MSG
-        node.name_end_location,
-        "Use each instead of each_with_object"
     end
 
     private def with_index_arg?(block : Crystal::Block)

src/ameba/rule/lint/shadowed_argument.cr

@@ -35,7 +35,6 @@ module Ameba::Rule::Lint
   # Lint/ShadowedArgument:
   #   Enabled: true
   # ```
-  #
   struct ShadowedArgument < Base
     properties do
       description "Disallows shadowed arguments"

src/ameba/rule/lint/shadowed_exception.cr

@@ -33,7 +33,6 @@ module Ameba::Rule::Lint
   # Lint/ShadowedException:
   #   Enabled: true
   # ```
-  #
   struct ShadowedException < Base
     properties do
       description "Disallows rescued exception that get shadowed"

src/ameba/rule/lint/shadowing_local_outer_var.cr

@@ -30,7 +30,6 @@ module Ameba::Rule::Lint
   # Lint/ShadowingOuterLocalVar:
   #   Enabled: true
   # ```
-  #
   struct ShadowingOuterLocalVar < Base
     properties do
       description "Disallows the usage of the same name as outer local variables" \

src/ameba/rule/lint/shared_var_in_fiber.cr

@@ -49,7 +49,6 @@ module Ameba::Rule::Lint
   # Lint/SharedVarInFiber:
   #   Enabled: true
   # ```
-  #
   struct SharedVarInFiber < Base
     properties do
       description "Disallows shared variables in fibers."
@@ -77,7 +76,7 @@ module Ameba::Rule::Lint
       declared_in = variable.assignments.first?.try &.branch
 
       variable.assignments
-        .reject { |assign| assign.scope.spawn_block? }
+        .reject(&.scope.spawn_block?)
         .any? do |assign|
           assign.branch.try(&.in_loop?) && assign.branch != declared_in
         end

src/ameba/rule/lint/syntax.cr

@@ -18,7 +18,6 @@ module Ameba::Rule::Lint
   # rescue e : Exception
   # end
   # ```
-  #
   struct Syntax < Base
     properties do
       description "Reports invalid Crystal syntax"

src/ameba/rule/lint/unneeded_disable_directive.cr

@@ -24,7 +24,6 @@ module Ameba::Rule::Lint
   # Lint/UnneededDisableDirective
   #   Enabled: true
   # ```
-  #
   struct UnneededDisableDirective < Base
     properties do
       description "Reports unneeded disable directives in comments"
@@ -47,11 +46,12 @@ module Ameba::Rule::Lint
       return unless directive[:action] == "disable"
 
       directive[:rules].reject do |rule_name|
+        next if rule_name == self.name
         source.issues.any? do |issue|
           issue.rule.name == rule_name &&
             issue.disabled? &&
             issue_at_location?(source, issue, location)
-        end && rule_name != self.name
+        end
       end
     end
 

src/ameba/rule/lint/unreachable_code.cr

@@ -41,7 +41,6 @@ module Ameba::Rule::Lint
   # Lint/UnreachableCode:
   #   Enabled: true
   # ```
-  #
   struct UnreachableCode < Base
     include AST::Util
 

src/ameba/rule/lint/unused_argument.cr

@@ -24,7 +24,6 @@ module Ameba::Rule::Lint
   #   IgnoreBlocks: false
   #   IgnoreProcs: false
   # ```
-  #
   struct UnusedArgument < Base
     properties do
       description "Disallows unused arguments"

src/ameba/rule/lint/useless_assign.cr

@@ -25,7 +25,6 @@ module Ameba::Rule::Lint
   # Lint/UselessAssign:
   #   Enabled: true
   # ```
-  #
   struct UselessAssign < Base
     properties do
       description "Disallows useless variable assignments"

src/ameba/rule/lint/useless_condition_in_when.cr

@@ -30,7 +30,6 @@ module Ameba::Rule::Lint
   # Lint/UselessConditionInWhen:
   #   Enabled: true
   # ```
-  #
   struct UselessConditionInWhen < Base
     properties do
       description "Disallows useless conditions in when"
@@ -43,10 +42,7 @@ module Ameba::Rule::Lint
     # simple implementation in future.
     protected def check_node(source, when_node, cond)
       cond_s = cond.to_s
-      return if when_node
+      return if when_node.conds.none?(&.to_s.==(cond_s))
-                  .conds
-                  .map(&.to_s)
-                  .none? { |c| c == cond_s }
 
       issue_for cond, MSG
     end

src/ameba/rule/metrics/cyclomatic_complexity.cr

@@ -8,7 +8,6 @@ module Ameba::Rule::Metrics
   #   Enabled: true
   #   MaxComplexity: 10
   # ```
-  #
   struct CyclomaticComplexity < Base
     properties do
       description "Disallows methods with a cyclomatic complexity higher than `MaxComplexity`"
@@ -21,17 +20,18 @@ module Ameba::Rule::Metrics
       complexity = AST::CountingVisitor.new(node).count
 
       if complexity > max_complexity && (location = node.name_location)
-        issue_for(
+        issue_for location, def_name_end_location(node), MSG % {
-          location,
+          complexity, max_complexity,
-          def_name_end_location(node),
+        }
-          MSG % {complexity, max_complexity}
-        )
       end
     end
 
     private def def_name_end_location(node)
       return unless location = node.name_location
-      line_number, column_number = location.line_number, location.column_number
+
+      line_number, column_number =
+        location.line_number, location.column_number
+
       Crystal::Location.new(location.filename, line_number, column_number + node.name.size)
     end
   end

src/ameba/rule/performance/any_after_filter.cr

@@ -24,10 +24,9 @@ module Ameba::Rule::Performance
   #     - select
   #     - reject
   # ```
-  #
   struct AnyAfterFilter < Base
     ANY_NAME = "any?"
-    MSG      = "Use `#{ANY_NAME} {...}` instead of `%s {...}.#{ANY_NAME}`"
+    MSG      = "Use `any? {...}` instead of `%s {...}.any?`"
 
     properties do
       filter_names : Array(String) = %w(select reject)
@@ -36,9 +35,10 @@ module Ameba::Rule::Performance
 
     def test(source, node : Crystal::Call)
       return unless node.name == ANY_NAME && (obj = node.obj)
+      return unless obj.is_a?(Crystal::Call)
+      return if obj.block.nil? || !node.block.nil?
 
-      if node.block.nil? && obj.is_a?(Crystal::Call) &&
+      if filter_names.includes?(obj.name)
-         filter_names.includes?(obj.name) && !obj.block.nil?
         issue_for obj.name_location, node.name_end_location, MSG % obj.name
       end
     end

src/ameba/rule/performance/first_last_after_filter.cr

@@ -23,7 +23,6 @@ module Ameba::Rule::Performance
   #   FilterNames:
   #     - select
   # ```
-  #
   struct FirstLastAfterFilter < Base
     CALL_NAMES  = %w(first last first? last?)
     MSG         = "Use `find {...}` instead of `%s {...}.%s`"
@@ -45,10 +44,10 @@ module Ameba::Rule::Performance
 
     def test(source, node : Crystal::Call)
       return unless CALL_NAMES.includes?(node.name) && (obj = node.obj)
-      return if node.args.any?
+      return unless obj.is_a?(Crystal::Call)
+      return if obj.block.nil? || !node.block.nil? || node.args.any?
 
-      if node.block.nil? && obj.is_a?(Crystal::Call) &&
+      if filter_names.includes?(obj.name)
-         filter_names.includes?(obj.name) && !obj.block.nil?
         message = node.name.includes?(CALL_NAMES.first) ? MSG : MSG_REVERSE
         issue_for obj.name_location, node.name_end_location, message % {obj.name, node.name}
       end

src/ameba/rule/performance/size_after_filter.cr

@@ -30,10 +30,9 @@ module Ameba::Rule::Performance
   #     - select
   #     - reject
   # ```
-  #
   struct SizeAfterFilter < Base
     SIZE_NAME = "size"
-    MSG       = "Use `count {...}` instead of `%s {...}.#{SIZE_NAME}`."
+    MSG       = "Use `count {...}` instead of `%s {...}.size`."
 
     properties do
       filter_names : Array(String) = %w(select reject)
@@ -51,9 +50,10 @@ module Ameba::Rule::Performance
 
     def test(source, node : Crystal::Call)
       return unless node.name == SIZE_NAME && (obj = node.obj)
+      return unless obj.is_a?(Crystal::Call)
+      return if obj.block.nil?
 
-      if obj.is_a?(Crystal::Call) &&
+      if filter_names.includes?(obj.name)
-         filter_names.includes?(obj.name) && !obj.block.nil?
         issue_for obj.name_location, node.name_end_location, MSG % obj.name
       end
     end

src/ameba/rule/style/constant_names.cr

@@ -21,7 +21,6 @@ module Ameba::Rule::Style
   # Style/ConstantNames:
   #   Enabled: true
   # ```
-  #
   struct ConstantNames < Base
     properties do
       description "Enforces constant names to be in screaming case"
@@ -34,7 +33,7 @@ module Ameba::Rule::Style
         name = target.names.first
         expected = name.upcase
 
-        return if expected == name || name.camelcase == name
+        return if name.in?(expected, name.camelcase)
 
         issue_for target, MSG % {expected, name}
       end

src/ameba/rule/style/is_a_nil.cr

@@ -19,7 +19,6 @@ module Ameba::Rule::Style
   # Style/IsANil:
   #   Enabled: true
   # ```
-  #
   struct IsANil < Base
     properties do
       description "Disallows calls to `is_a?(Nil)` in favor of `nil?`"
@@ -32,7 +31,9 @@ module Ameba::Rule::Style
       return if node.nil_check?
 
       const = node.const
-      issue_for const, MSG if const.is_a?(Crystal::Path) && const.names == PATH_NIL_NAMES
+      return unless const.is_a?(Crystal::Path) && const.names == PATH_NIL_NAMES
+
+      issue_for const, MSG
     end
   end
 end

src/ameba/rule/style/large_numbers.cr

@@ -26,7 +26,6 @@ module Ameba::Rule::Style
   #   Enabled: true
   #   IntMinDigits: 5 # i.e. integers higher than 9999
   # ```
-  #
   struct LargeNumbers < Base
     properties do
       description "Disallows usage of large numbers without underscore"
@@ -53,7 +52,7 @@ module Ameba::Rule::Style
     end
 
     private def allowed?(_sign, value, fraction, _suffix)
-      return true if !fraction.nil? && fraction.size > 3
+      return true if fraction && fraction.size > 3
 
       digits = value.chars.select &.to_s.=~ /[0-9]/
       digits.size >= int_min_digits
@@ -71,7 +70,7 @@ module Ameba::Rule::Style
         value.chars.reject(&.== '_').each_slice(by) do |slice|
           slices << (yield slice).join
         end
-      end.join("_")
+      end.join('_')
     end
 
     private def parse_number(value)

src/ameba/rule/style/method_names.cr

@@ -37,7 +37,6 @@ module Ameba::Rule::Style
   # Style/MethodNames:
   #   Enabled: true
   # ```
-  #
   struct MethodNames < Base
     properties do
       description "Enforces method names to be in underscored case"
@@ -51,7 +50,7 @@ module Ameba::Rule::Style
       line_number = node.location.try &.line_number
       column_number = node.name_location.try &.column_number
 
-      return if line_number.nil? || column_number.nil?
+      return unless line_number && column_number
 
       issue_for(
         {line_number, column_number},

src/ameba/rule/style/negated_conditions_in_unless.cr

@@ -26,7 +26,6 @@ module Ameba::Rule::Style
   # Style/NegatedConditionsInUnless:
   #   Enabled: true
   # ```
-  #
   struct NegatedConditionsInUnless < Base
     properties do
       description "Disallows negated conditions in unless"
@@ -35,8 +34,7 @@ module Ameba::Rule::Style
     MSG = "Avoid negated conditions in unless blocks"
 
     def test(source, node : Crystal::Unless)
-      return unless negated_condition? node.cond
+      issue_for node, MSG if negated_condition?(node.cond)
-      issue_for node, MSG
     end
 
     private def negated_condition?(node)

src/ameba/rule/style/predicate_name.cr

@@ -28,7 +28,6 @@ module Ameba::Rule::Style
   # Style/PredicateName:
   #   Enabled: true
   # ```
-  #
   struct PredicateName < Base
     properties do
       description "Disallows tautological predicate names"

src/ameba/rule/style/redundant_begin.cr

@@ -55,7 +55,6 @@ module Ameba::Rule::Style
   # Style/RedundantBegin:
   #   Enabled: true
   # ```
-  #
   struct RedundantBegin < Base
     include AST::Util
     properties do
@@ -65,9 +64,7 @@ module Ameba::Rule::Style
     MSG = "Redundant `begin` block detected"
 
     def test(source, node : Crystal::Def)
-      return unless redundant_begin?(source, node)
+      issue_for node, MSG if redundant_begin?(source, node)
-
-      issue_for node, MSG
     end
 
     private def redundant_begin?(source, node)

src/ameba/rule/style/type_names.cr

@@ -51,7 +51,6 @@ module Ameba::Rule::Style
   # Style/TypeNames:
   #   Enabled: true
   # ```
-  #
   struct TypeNames < Base
     properties do
       description "Enforces type names in camelcase manner"
@@ -62,7 +61,7 @@ module Ameba::Rule::Style
     private def check_node(source, node)
       name = node.name.to_s
       expected = name.camelcase
-      return if expected == name
+      return if name == expected
 
       issue_for node, MSG % {expected, name}
     end

src/ameba/rule/style/unless_else.cr

@@ -42,7 +42,6 @@ module Ameba::Rule::Style
   # Style/UnlessElse:
   #   Enabled: true
   # ```
-  #
   struct UnlessElse < Base
     properties do
       description "Disallows the use of an `else` block with the `unless`"
@@ -51,8 +50,7 @@ module Ameba::Rule::Style
     MSG = "Favour if over unless with else"
 
     def test(source, node : Crystal::Unless)
-      return if node.else.nop?
+      issue_for node, MSG unless node.else.nop?
-      issue_for node, MSG
     end
   end
 end

src/ameba/rule/style/variable_names.cr

@@ -22,7 +22,6 @@ module Ameba::Rule::Style
   # Style/VariableNames:
   #   Enabled: true
   # ```
-  #
   struct VariableNames < Base
     properties do
       description "Enforces variable names to be in underscored case"

src/ameba/rule/style/while_true.cr

@@ -25,7 +25,6 @@ module Ameba::Rule::Style
   # Style/WhileTrue:
   #   Enabled: true
   # ```
-  #
   struct WhileTrue < Base
     properties do
       description "Disallows while statements with a true literal as condition"
@@ -34,8 +33,7 @@ module Ameba::Rule::Style
     MSG = "While statement using true literal as condition"
 
     def test(source, node : Crystal::While)
-      return unless node.cond.true_literal?
+      issue_for node, MSG if node.cond.true_literal?
-      issue_for node, MSG
     end
   end
 end