From 19c370aee06a6c7bbaec7c42b1d3331f77256fc5 Mon Sep 17 00:00:00 2001
From: Sijawusz Pur Rahnama <sija@sija.pl>
Date: Mon, 11 Jan 2021 19:13:58 +0100
Subject: [PATCH] Misc refactors (#180)

* Optimize Severity#symbol

* Remove empty else branches

* Optimize map+compact/flatten calls

* Misc stylistic refactors
---
 src/ameba/ast/branchable.cr                 |  3 +--
 src/ameba/ast/flow_expression.cr            |  2 --
 src/ameba/ast/scope.cr                      | 10 +++----
 src/ameba/cli/cmd.cr                        | 29 +++++++++++++--------
 src/ameba/config.cr                         | 15 ++++++-----
 src/ameba/formatter/dot_formatter.cr        |  9 ++++---
 src/ameba/formatter/explain_formatter.cr    | 15 +++++------
 src/ameba/formatter/flycheck_formatter.cr   |  2 +-
 src/ameba/formatter/todo_formatter.cr       | 18 ++++++-------
 src/ameba/formatter/util.cr                 |  6 ++---
 src/ameba/glob_utils.cr                     |  4 +--
 src/ameba/inline_comments.cr                |  6 +++--
 src/ameba/rule/base.cr                      |  8 +++---
 src/ameba/rule/lint/percent_array.cr        |  4 ---
 src/ameba/rule/lint/redundant_with_index.cr |  2 --
 src/ameba/rule/style/redundant_begin.cr     |  4 +--
 src/ameba/runner.cr                         |  6 ++---
 src/ameba/severity.cr                       |  8 ++++--
 src/ameba/source.cr                         | 25 +++++++-----------
 src/ameba/tokenizer.cr                      |  9 +------
 20 files changed, 87 insertions(+), 98 deletions(-)

diff --git a/src/ameba/ast/branchable.cr b/src/ameba/ast/branchable.cr
index 2495dbb2..4bf9819e 100644
--- a/src/ameba/ast/branchable.cr
+++ b/src/ameba/ast/branchable.cr
@@ -37,8 +37,7 @@ module Ameba::AST
 
     # Returns true if this node or one of the parent branchables is a loop, false otherwise.
     def loop?
-      return true if loop?(node)
-      parent.try(&.loop?) || false
+      loop?(node) || parent.try(&.loop?) || false
     end
   end
 end
diff --git a/src/ameba/ast/flow_expression.cr b/src/ameba/ast/flow_expression.cr
index 6b214962..8c80eb53 100644
--- a/src/ameba/ast/flow_expression.cr
+++ b/src/ameba/ast/flow_expression.cr
@@ -59,8 +59,6 @@ module Ameba::AST
         end
       when Crystal::BinaryOp
         unreachable_nodes << current_node.right if flow_expression?(current_node.left, in_loop?)
-      else
-        # nop
       end
 
       unreachable_nodes
diff --git a/src/ameba/ast/scope.cr b/src/ameba/ast/scope.cr
index 3627ef73..a6a8ee08 100644
--- a/src/ameba/ast/scope.cr
+++ b/src/ameba/ast/scope.cr
@@ -117,8 +117,8 @@ module Ameba::AST
 
     # Returns true instance variable assinged in this scope.
     def assigns_ivar?(name)
-      arguments.find { |arg| arg.name == name } &&
-        ivariables.find { |var| var.name == "@#{name}" }
+      arguments.find(&.name.== name) &&
+        ivariables.find(&.name.== "@#{name}")
     end
 
     # Returns true if and only if current scope represents some
@@ -137,13 +137,13 @@ module Ameba::AST
     def references?(variable : Variable)
       variable.references.any? do |reference|
         reference.scope == self ||
-          inner_scopes.any?(&.references? variable)
+          inner_scopes.any?(&.references?(variable))
       end || variable.used_in_macro?
     end
 
     # Returns true if current scope is a def, false if not.
     def def?
-      node.is_a? Crystal::Def
+      node.is_a?(Crystal::Def)
     end
 
     # Returns true if this scope is a top level scope, false if not.
@@ -173,7 +173,7 @@ module Ameba::AST
     # the same Crystal node as `@node`.
     def eql?(node)
       node == @node &&
-        !node.location.nil? &&
+        node.location &&
         node.location == @node.location
     end
   end
diff --git a/src/ameba/cli/cmd.cr b/src/ameba/cli/cmd.cr
index e34ba241..7b9e6bae 100644
--- a/src/ameba/cli/cmd.cr
+++ b/src/ameba/cli/cmd.cr
@@ -72,12 +72,12 @@ module Ameba::Cli
 
       parser.on("--only RULE1,RULE2,...",
         "Run only given rules (or groups)") do |rules|
-        opts.only = rules.split ","
+        opts.only = rules.split(',')
       end
 
       parser.on("--except RULE1,RULE2,...",
         "Disable the given rules (or groups)") do |rules|
-        opts.except = rules.split ","
+        opts.except = rules.split(',')
       end
 
       parser.on("--all", "Enables all available rules") do
@@ -90,7 +90,8 @@ module Ameba::Cli
         opts.config = ""
       end
 
-      parser.on("--fail-level SEVERITY", "Change the level of failure to exit. Defaults to Convention") do |level|
+      parser.on("--fail-level SEVERITY",
+        "Change the level of failure to exit. Defaults to Convention") do |level|
         opts.fail_level = Severity.parse(level)
       end
 
@@ -113,13 +114,13 @@ module Ameba::Cli
   end
 
   private def configure_rules(config, opts)
-    if only = opts.only
-      config.rules.map! { |r| r.enabled = false; r }
+    case
+    when only = opts.only
+      config.rules.map! &.tap(&.enabled = false)
       config.update_rules(only, enabled: true)
-    elsif opts.all?
-      config.rules.map! { |r| r.enabled = true; r }
+    when opts.all?
+      config.rules.map! &.tap(&.enabled = true)
     end
-
     config.update_rules(opts.except, enabled: false)
   end
 
@@ -127,7 +128,8 @@ module Ameba::Cli
     if name = opts.formatter
       config.formatter = name
     end
-    config.formatter.config[:without_affected_code] = opts.without_affected_code?
+    config.formatter.config[:without_affected_code] =
+      opts.without_affected_code?
   end
 
   private def configure_explain_opts(loc, opts)
@@ -138,10 +140,15 @@ module Ameba::Cli
   end
 
   private def parse_explain_location(arg)
-    location = arg.split(":", remove_empty: true).map &.strip
+    location = arg.split(':', remove_empty: true).map! &.strip
     raise ArgumentError.new unless location.size === 3
+
     file, line, column = location
-    {file: file, line: line.to_i, column: column.to_i}
+    {
+      file:   file,
+      line:   line.to_i,
+      column: column.to_i,
+    }
   rescue
     raise "location should have PATH:line:column format"
   end
diff --git a/src/ameba/config.cr b/src/ameba/config.cr
index 1ecbd3a8..5bf0261e 100644
--- a/src/ameba/config.cr
+++ b/src/ameba/config.cr
@@ -31,7 +31,6 @@ class Ameba::Config
     !lib
   )
 
-  setter formatter : Formatter::BaseFormatter?
   getter rules : Array(Rule::Base)
   property severity = Severity::Convention
 
@@ -66,7 +65,9 @@ class Ameba::Config
     @excluded = load_array_section(config, "Excluded")
     @globs = load_array_section(config, "Globs", DEFAULT_GLOBS)
 
-    self.formatter = load_formatter_name(config)
+    if formatter_name = load_formatter_name(config)
+      self.formatter = formatter_name
+    end
   end
 
   # Loads YAML configuration file by `path`.
@@ -84,7 +85,7 @@ class Ameba::Config
   end
 
   def self.formatter_names
-    AVAILABLE_FORMATTERS.keys.join("|")
+    AVAILABLE_FORMATTERS.keys.join('|')
   end
 
   # Returns a list of sources matching globs and excluded sections.
@@ -111,8 +112,8 @@ class Ameba::Config
   # config.formatter
   # ```
   #
-  def formatter
-    @formatter ||= Formatter::DotFormatter.new
+  property formatter : Formatter::BaseFormatter do
+    Formatter::DotFormatter.new
   end
 
   # Sets formatter by name.
@@ -138,7 +139,7 @@ class Ameba::Config
   # ```
   #
   def update_rule(name, enabled = true, excluded = nil)
-    index = @rules.index { |r| r.name == name }
+    index = @rules.index { |rule| rule.name == name }
     raise ArgumentError.new("Rule `#{name}` does not exist") unless index
 
     rule = @rules[index]
@@ -172,7 +173,7 @@ class Ameba::Config
 
   private def load_formatter_name(config)
     name = config["Formatter"]?.try &.["Name"]?
-    name ? name.to_s : nil
+    name.try(&.to_s)
   end
 
   private def load_array_section(config, section_name, default = [] of String)
diff --git a/src/ameba/formatter/dot_formatter.cr b/src/ameba/formatter/dot_formatter.cr
index 396069e5..54fe53ac 100644
--- a/src/ameba/formatter/dot_formatter.cr
+++ b/src/ameba/formatter/dot_formatter.cr
@@ -35,14 +35,17 @@ module Ameba::Formatter
           next if issue.disabled?
           next if (location = issue.location).nil?
 
-          output << "#{location}\n".colorize(:cyan)
-          output << "[#{issue.rule.severity.symbol}] #{issue.rule.name}: #{issue.message}\n".colorize(:red)
+          output.puts location.colorize(:cyan)
+          output.puts \
+            "[#{issue.rule.severity.symbol}] " \
+            "#{issue.rule.name}: " \
+            "#{issue.message}".colorize(:red)
 
           if show_affected_code && (code = affected_code(source, location))
             output << code.colorize(:default)
           end
 
-          output << "\n"
+          output << '\n'
         end
       end
 
diff --git a/src/ameba/formatter/explain_formatter.cr b/src/ameba/formatter/explain_formatter.cr
index 6670245b..9220ae3f 100644
--- a/src/ameba/formatter/explain_formatter.cr
+++ b/src/ameba/formatter/explain_formatter.cr
@@ -4,9 +4,8 @@ module Ameba::Formatter
   # A formatter that shows the detailed explanation of the issue at
   # a specific location.
   class ExplainFormatter
-    LINE_BREAK = "\n"
-    HEADING    = "## "
-    PREFIX     = " "
+    HEADING = "## "
+    PREFIX  = " "
 
     include Util
 
@@ -65,19 +64,19 @@ module Ameba::Formatter
     end
 
     private def output_title(title)
-      output << HEADING.colorize(:yellow) << title.colorize(:yellow) << LINE_BREAK
-      output << LINE_BREAK
+      output << HEADING.colorize(:yellow) << title.colorize(:yellow) << '\n'
+      output << '\n'
     end
 
     private def output_paragraph(paragraph : String)
-      output_paragraph(paragraph.split(LINE_BREAK))
+      output_paragraph(paragraph.split('\n'))
     end
 
     private def output_paragraph(paragraph : Array(String))
       paragraph.each do |line|
-        output << PREFIX << line << LINE_BREAK
+        output << PREFIX << line << '\n'
       end
-      output << LINE_BREAK
+      output << '\n'
     end
   end
 end
diff --git a/src/ameba/formatter/flycheck_formatter.cr b/src/ameba/formatter/flycheck_formatter.cr
index 89423ff3..50d5de6c 100644
--- a/src/ameba/formatter/flycheck_formatter.cr
+++ b/src/ameba/formatter/flycheck_formatter.cr
@@ -9,7 +9,7 @@ module Ameba::Formatter
           @mutex.synchronize do
             output.printf "%s:%d:%d: %s: [%s] %s\n",
               source.path, loc.line_number, loc.column_number, e.rule.severity.symbol,
-              e.rule.name, e.message.gsub("\n", " ")
+              e.rule.name, e.message.gsub('\n', " ")
           end
         end
       end
diff --git a/src/ameba/formatter/todo_formatter.cr b/src/ameba/formatter/todo_formatter.cr
index fd0b8100..88584d1d 100644
--- a/src/ameba/formatter/todo_formatter.cr
+++ b/src/ameba/formatter/todo_formatter.cr
@@ -8,19 +8,20 @@ module Ameba::Formatter
 
     def finished(sources)
       super
-      issues = sources.map(&.issues).flatten
+
+      issues = sources.flat_map(&.issues)
       unless issues.any? { |issue| !issue.disabled? }
-        @output << "No issues found. File is not generated.\n"
+        @output.puts "No issues found. File is not generated."
         return
       end
 
-      if issues.any? { |issue| issue.syntax? }
-        @output << "Unable to generate TODO file. Please fix syntax issues.\n"
+      if issues.any?(&.syntax?)
+        @output.puts "Unable to generate TODO file. Please fix syntax issues."
         return
       end
 
       file = generate_todo_config issues
-      @output << "Created #{file.path}\n"
+      @output.puts "Created #{file.path}"
       file
     end
 
@@ -57,10 +58,9 @@ module Ameba::Formatter
     end
 
     private def rule_todo(rule, issues)
-      rule.excluded =
-        issues.map(&.location.try &.filename.try &.to_s)
-          .compact
-          .uniq!
+      rule.excluded = issues
+        .compact_map(&.location.try &.filename.try &.to_s)
+        .uniq!
 
       {rule.name => rule}.to_yaml
     end
diff --git a/src/ameba/formatter/util.cr b/src/ameba/formatter/util.cr
index 7219a67b..26410313 100644
--- a/src/ameba/formatter/util.cr
+++ b/src/ameba/formatter/util.cr
@@ -2,9 +2,9 @@ module Ameba::Formatter
   module Util
     def affected_code(source, location, max_length = 100, placeholder = " ...", prompt = "> ")
       line, column = location.line_number, location.column_number
-      affected_line = source.lines[line - 1]?
+      affected_line = source.lines[line - 1]?.presence
 
-      return if affected_line.nil? || affected_line.strip.empty?
+      return unless affected_line
 
       if affected_line.size > max_length && column < max_length
         affected_line = affected_line[0, max_length - placeholder.size - 1] + placeholder
@@ -14,7 +14,7 @@ module Ameba::Formatter
       position = column - (affected_line.size - stripped.size) + prompt.size
 
       String.build do |str|
-        str << prompt << stripped << "\n"
+        str << prompt << stripped << '\n'
         str << " " * (position - 1)
         str << "^".colorize(:yellow)
       end
diff --git a/src/ameba/glob_utils.cr b/src/ameba/glob_utils.cr
index a714f885..b014bf38 100644
--- a/src/ameba/glob_utils.cr
+++ b/src/ameba/glob_utils.cr
@@ -22,10 +22,10 @@ module Ameba
     # ```
     #
     def expand(globs)
-      globs.map do |glob|
+      globs.flat_map do |glob|
         glob += "/**/*.cr" if File.directory?(glob)
         Dir[glob]
-      end.flatten.uniq!
+      end.uniq!
     end
 
     private def rejected_globs(globs)
diff --git a/src/ameba/inline_comments.cr b/src/ameba/inline_comments.cr
index cece57fe..7d1ec14d 100644
--- a/src/ameba/inline_comments.cr
+++ b/src/ameba/inline_comments.cr
@@ -89,8 +89,10 @@ module Ameba
 
     private def line_disabled?(line, rule)
       return false unless directive = parse_inline_directive(line)
-      Action.parse?(directive[:action]).try(&.disable?) &&
-        (directive[:rules].includes?(rule.name) || directive[:rules].includes?(rule.group))
+      return false unless Action.parse?(directive[:action]).try(&.disable?)
+
+      directive[:rules].includes?(rule.name) ||
+        directive[:rules].includes?(rule.group)
     end
 
     private def commented_out?(line)
diff --git a/src/ameba/rule/base.cr b/src/ameba/rule/base.cr
index c895499a..de4c363f 100644
--- a/src/ameba/rule/base.cr
+++ b/src/ameba/rule/base.cr
@@ -95,7 +95,7 @@ module Ameba::Rule
     def excluded?(source)
       excluded.try &.any? do |path|
         source.matches_path?(path) ||
-          Dir.glob(path).any? { |glob| source.matches_path? glob }
+          Dir.glob(path).any? { |glob| source.matches_path?(glob) }
       end
     end
 
@@ -123,11 +123,11 @@ module Ameba::Rule
     end
 
     protected def self.rule_name
-      name.gsub("Ameba::Rule::", "").gsub("::", "/")
+      name.gsub("Ameba::Rule::", "").gsub("::", '/')
     end
 
     protected def self.group_name
-      rule_name.split("/")[0...-1].join("/")
+      rule_name.split('/')[0...-1].join('/')
     end
 
     protected def self.subclasses
@@ -157,7 +157,7 @@ module Ameba::Rule
     def self.parsed_doc
       source = File.read(path_to_source_file)
       nodes = Crystal::Parser.new(source).tap(&.wants_doc = true).parse
-      type_name = rule_name.split("/").last?
+      type_name = rule_name.split('/').last?
       DocFinder.new(nodes, type_name).doc
     end
 
diff --git a/src/ameba/rule/lint/percent_array.cr b/src/ameba/rule/lint/percent_array.cr
index 0f1d3de8..afdf0223 100644
--- a/src/ameba/rule/lint/percent_array.cr
+++ b/src/ameba/rule/lint/percent_array.cr
@@ -49,8 +49,6 @@ module Ameba::Rule::Lint
             issue_for start_token.not_nil!, issue.not_nil!
           end
           issue = start_token = nil
-        else
-          # nop
         end
       end
     end
@@ -61,8 +59,6 @@ module Ameba::Rule::Lint
         check_array_entry entry, string_array_unwanted_symbols, "%w"
       when .starts_with? "%i"
         check_array_entry entry, symbol_array_unwanted_symbols, "%i"
-      else
-        # nop
       end
     end
 
diff --git a/src/ameba/rule/lint/redundant_with_index.cr b/src/ameba/rule/lint/redundant_with_index.cr
index f67766d6..d9c646f3 100644
--- a/src/ameba/rule/lint/redundant_with_index.cr
+++ b/src/ameba/rule/lint/redundant_with_index.cr
@@ -42,8 +42,6 @@ module Ameba::Rule::Lint
         report source, node, "Remove redundant with_index"
       when "each_with_index"
         report source, node, "Use each instead of each_with_index"
-      else
-        # nop
       end
     end
 
diff --git a/src/ameba/rule/style/redundant_begin.cr b/src/ameba/rule/style/redundant_begin.cr
index bfb028a6..1318bb4b 100644
--- a/src/ameba/rule/style/redundant_begin.cr
+++ b/src/ameba/rule/style/redundant_begin.cr
@@ -76,8 +76,6 @@ module Ameba::Rule::Style
         redundant_begin_in_handler?(source, body, node)
       when Crystal::Expressions
         redundant_begin_in_expressions?(body)
-      else
-        # nop
       end
     end
 
@@ -88,7 +86,7 @@ module Ameba::Rule::Style
     private def redundant_begin_in_handler?(source, handler, node)
       return false if begin_exprs_in_handler?(handler) || inner_handler?(handler)
 
-      code = node_source(node, source.lines).try &.join("\n")
+      code = node_source(node, source.lines).try &.join('\n')
       def_redundant_begin? code if code
     rescue
       false
diff --git a/src/ameba/runner.cr b/src/ameba/runner.cr
index ece39187..e742dab2 100644
--- a/src/ameba/runner.cr
+++ b/src/ameba/runner.cr
@@ -50,7 +50,6 @@ module Ameba
           .find &.name.==(Rule::Lint::UnneededDisableDirective.rule_name)
     end
 
-    # :nodoc:
     protected def initialize(@rules, @sources, @formatter, @severity)
     end
 
@@ -80,9 +79,8 @@ module Ameba
         end
       end
 
-      channels.each do |c|
-        e = c.receive
-        raise e unless e.nil?
+      channels.each do |chan|
+        chan.receive.try { |e| raise e }
       end
 
       self
diff --git a/src/ameba/severity.cr b/src/ameba/severity.cr
index 917cf4e9..3b0e4327 100644
--- a/src/ameba/severity.cr
+++ b/src/ameba/severity.cr
@@ -9,8 +9,12 @@ module Ameba
     # ```
     # Severity::Warning.symbol # => 'W'
     # ```
-    def symbol
-      to_s[0]
+    def symbol : Char
+      case self
+      in Error      then 'E'
+      in Warning    then 'W'
+      in Convention then 'C'
+      end
     end
 
     # Creates Severity by the name.
diff --git a/src/ameba/source.cr b/src/ameba/source.cr
index 3fd8d751..70d38627 100644
--- a/src/ameba/source.cr
+++ b/src/ameba/source.cr
@@ -11,10 +11,6 @@ module Ameba
     # Crystal code (content of a source file).
     getter code : String
 
-    @lines : Array(String)?
-    @ast : Crystal::ASTNode?
-    @fullpath : String?
-
     # Creates a new source by `code` and `path`.
     #
     # For example:
@@ -24,7 +20,7 @@ module Ameba
     # Ameba::Source.new File.read(path), path
     # ```
     #
-    def initialize(@code : String, @path = "")
+    def initialize(@code, @path = "")
     end
 
     # Returns lines of code splitted by new line character.
@@ -38,9 +34,7 @@ module Ameba
     # source.lines # => ["a = 1", "b = 2"]
     # ```
     #
-    def lines
-      @lines ||= @code.split("\n")
-    end
+    getter lines : Array(String) { code.split('\n') }
 
     # Returns AST nodes constructed by `Crystal::Parser`.
     #
@@ -49,16 +43,15 @@ module Ameba
     # source.ast
     # ```
     #
-    def ast
-      @ast ||=
-        Crystal::Parser.new(code)
-          .tap { |parser| parser.wants_doc = true }
-          .tap { |parser| parser.filename = @path }
-          .parse
+    getter ast : Crystal::ASTNode do
+      Crystal::Parser.new(code)
+        .tap(&.wants_doc = true)
+        .tap(&.filename = path)
+        .parse
     end
 
-    def fullpath
-      @fullpath ||= File.expand_path @path
+    getter fullpath : String do
+      File.expand_path(path)
     end
 
     # Returns true if *filepath* matches the source's path, false if it does not.
diff --git a/src/ameba/tokenizer.cr b/src/ameba/tokenizer.cr
index 070e5b55..e79d12cb 100644
--- a/src/ameba/tokenizer.cr
+++ b/src/ameba/tokenizer.cr
@@ -53,8 +53,7 @@ module Ameba
       false
     end
 
-    private def run_normal_state(lexer, break_on_rcurly = false,
-                                 &block : Crystal::Token -> _)
+    private def run_normal_state(lexer, break_on_rcurly = false, &block : Crystal::Token -> _)
       loop do
         token = @lexer.next_token
         block.call token
@@ -68,8 +67,6 @@ module Ameba
           break
         when :"}"
           break if break_on_rcurly
-        else
-          # go on
         end
       end
     end
@@ -86,8 +83,6 @@ module Ameba
           run_normal_state lexer, break_on_rcurly: true, &block
         when :EOF
           break
-        else
-          # go on
         end
       end
     end
@@ -102,8 +97,6 @@ module Ameba
           break
         when :EOF
           break
-        else
-          # go on
         end
       end
     end