Unnecessary If/Unless (#89)

README.md

@@ -18,24 +18,25 @@ on `sourceror`.
 > mix recode.help
 
 Design tasks:
-TagFIXME          # Checker   - Checks if there are FIXME tags in the sources.
-TagTODO           # Checker   - Checks if there are TODO tags in the sources.
-Readability tasks:
-AliasExpansion    # Corrector - Exapnds multi aliases to separate aliases.
-AliasOrder        # Corrector - Checks if aliases are sorted alphabetically.
-EnforceLineLength # Corrector - Forces expressions to one line.
-Format            # Corrector - Does the same as `mix format`.
-PipeFunOne        # Corrector - Add parentheses to one-arity functions.
-SinglePipe        # Corrector - Pipes should only be used when piping data through multiple calls.
-Specs             # Checker   - Checks for specs.
+TagFIXME            # Checker   - Checks if there are FIXME tags in the sources.
+TagTODO             # Checker   - Checks if there are TODO tags in the sources.
+Readability tasks:  
+AliasExpansion      # Corrector - Exapnds multi aliases to separate aliases.
+AliasOrder          # Corrector - Checks if aliases are sorted alphabetically.
+EnforceLineLength   # Corrector - Forces expressions to one line.
+Format              # Corrector - Does the same as `mix format`.
+PipeFunOne          # Corrector - Add parentheses to one-arity functions.
+SinglePipe          # Corrector - Pipes should only be used when piping data through multiple calls.
+Specs               # Checker   - Checks for specs.
 Refactor tasks:
-FilterCount       # Corrector - Checks calls like Enum.filter(...) |> Enum.count().
-Nesting           # Checker   - Checks code nesting depth in functions and macros.
+FilterCount         # Corrector - Checks calls like Enum.filter(...) |> Enum.count().
+Nesting             # Checker   - Checks code nesting depth in functions and macros.
+UnnecessaryIfUnless # Corrector - Changes `if expr, do: true, else: false` to `expr`
 Warning tasks:
-Dbg               # Corrector - There should be no calls to dbg.
-IOInspect         # Corrector - There should be no calls to IO.inspect.
-TestFileExt       # Corrector - Checks the file extension of test files.
-UnusedVariable    # Corrector - Checks if unused variables occur.
+Dbg                 # Corrector - There should be no calls to dbg.
+IOInspect           # Corrector - There should be no calls to IO.inspect.
+TestFileExt         # Corrector - Checks the file extension of test files.
+UnusedVariable      # Corrector - Checks if unused variables occur.
 ```
 
 It is also possible to run `recode` in a none-autocorrect mode to just lint your
@@ -114,6 +115,7 @@ This mix task generates the config file `.recode.exs`.
     {Recode.Task.TagFIXME, [exit_code: 2]},
     {Recode.Task.TagTODO, [exit_code: 4]},
     {Recode.Task.TestFileExt, []},
+    {Recode.Task.UnnecessaryIfUnless, []},
     {Recode.Task.UnusedVariable, [active: false]}
   ]
 ]

lib/recode/config.ex

@@ -42,6 +42,7 @@ defmodule Recode.Config do
       {Recode.Task.TagFIXME, exit_code: 2},
       {Recode.Task.TagTODO, exit_code: 4},
       {Recode.Task.TestFileExt, []},
+      {Recode.Task.UnnecessaryIfUnless, []},
       {Recode.Task.UnusedVariable, [active: false]}
     ]
   ]

lib/recode/task/unnecessary_if_unless.ex

@@ -0,0 +1,110 @@
+defmodule Recode.Task.UnnecessaryIfUnless do
+  @shortdoc "Removes redundant booleans"
+
+  @moduledoc """
+  Redudant booleans make code needlesly verbose.
+
+          # preferred
+          foo == bar
+
+          # not preferred
+          if foo == bar do
+            true
+          else
+            false
+          end
+
+  This task rewrites the code when `mix recode` runs with `autocorrect: true`.
+  """
+
+  use Recode.Task, corrector: true, category: :readability
+
+  alias Recode.Issue
+  alias Recode.Task.UnnecessaryIfUnless
+  alias Rewrite.Source
+  alias Sourceror.Zipper
+
+  @impl Recode.Task
+  def run(source, opts) do
+    {zipper, issues} =
+      source
+      |> Source.get(:quoted)
+      |> Zipper.zip()
+      |> Zipper.traverse([], fn zipper, issues ->
+        remove_unecessary_if_unless(zipper, issues, opts[:autocorrect])
+      end)
+
+    case opts[:autocorrect] do
+      true ->
+        Source.update(source, UnnecessaryIfUnless, :quoted, Zipper.root(zipper))
+
+      false ->
+        Source.add_issues(source, issues)
+    end
+  end
+
+  defp remove_unecessary_if_unless(
+         %Zipper{node: {conditional, meta, body}} = zipper,
+         issues,
+         true
+       )
+       when conditional in [:if, :unless] do
+    case extract(body, conditional) do
+      {:ok, expr} ->
+        expr = put_leading_comments(expr, meta)
+
+        {Zipper.replace(zipper, expr), issues}
+
+      :error ->
+        {zipper, issues}
+    end
+  end
+
+  defp remove_unecessary_if_unless(
+         %Zipper{node: {conditional, meta, body}} = zipper,
+         issues,
+         false
+       )
+       when conditional in [:if, :unless] do
+    issues =
+      case extract(body, conditional) do
+        {:ok, _expr} ->
+          message = "Avoid unnecessary `if` and `unless`"
+          issue = Issue.new(UnnecessaryIfUnless, message, meta)
+          [issue | issues]
+
+        :error ->
+          issues
+      end
+
+    {zipper, issues}
+  end
+
+  defp remove_unecessary_if_unless(zipper, issues, _autocorrect), do: {zipper, issues}
+
+  defp extract(
+         [
+           expr,
+           [
+             {{:__block__, _, [:do]}, {:__block__, _, [left]}},
+             {{:__block__, _, [:else]}, {:__block__, _, [right]}}
+           ]
+         ],
+         conditional
+       )
+       when is_boolean(left) and is_boolean(right) do
+    case {conditional, left, right} do
+      {:if, true, false} -> {:ok, expr}
+      {:if, false, true} -> {:ok, {:not, [], [expr]}}
+      {:unless, true, false} -> {:ok, {:not, [], [expr]}}
+      {:unless, false, true} -> {:ok, expr}
+    end
+  end
+
+  defp extract(_, _), do: :error
+
+  defp put_leading_comments(expr, meta) do
+    comments = meta[:leading_comments] || []
+    Sourceror.append_comments(expr, comments)
+  end
+end

mix.exs

@@ -61,6 +61,7 @@ defmodule Recode.MixProject do
           Recode.Task.TagFIXME,
           Recode.Task.TagTODO,
           Recode.Task.TestFileExt,
+          Recode.Task.UnnecessaryIfInless,
           Recode.Task.UnusedVariable
         ]
       ]

test/recode/task/unnecessary_if_unless_test.exs

@@ -0,0 +1,173 @@
+defmodule Recode.Task.UnnecessaryIfUnlessTest do
+  use RecodeCase
+
+  alias Recode.Task.UnnecessaryIfUnless
+
+  describe "run/1" do
+    test "equals" do
+      code = """
+      if foo == bar do
+        true
+      else
+        false
+      end
+      """
+
+      expected = """
+      foo == bar
+      """
+
+      code
+      |> run_task(UnnecessaryIfUnless, autocorrect: true)
+      |> assert_code(expected)
+    end
+
+    test "predicate" do
+      code = """
+      if foo?(bar) do
+        true
+      else
+        false
+      end
+      """
+
+      expected = """
+      foo?(bar)
+      """
+
+      code
+      |> run_task(UnnecessaryIfUnless, autocorrect: true)
+      |> assert_code(expected)
+    end
+
+    test "keyword syntax" do
+      code = """
+      if foo?(bar), do: true, else: false
+      """
+
+      expected = """
+      foo?(bar)
+      """
+
+      code
+      |> run_task(UnnecessaryIfUnless, autocorrect: true)
+      |> assert_code(expected)
+    end
+
+    test "negates code with reverse booleans" do
+      code = """
+      if some_call?() && (some_other_call?() || another_call?()) do
+        false
+      else
+        true
+      end
+      """
+
+      expected = """
+      not (some_call?() && (some_other_call?() || another_call?()))
+      """
+
+      code
+      |> run_task(UnnecessaryIfUnless, autocorrect: true)
+      |> assert_code(expected)
+    end
+
+    test "maintains leading comments" do
+      code = """
+      # I'm a comment
+      if foo?(bar) do
+        true
+      else
+        false
+      end
+      """
+
+      expected = """
+      # I'm a comment
+      foo?(bar)
+      """
+
+      code
+      |> run_task(UnnecessaryIfUnless, autocorrect: true)
+      |> assert_code(expected)
+    end
+
+    test "keeps code as is without booleans" do
+      code = """
+      if foo?(bar) do
+        bar
+      else
+        false
+      end
+      """
+
+      expected = """
+      if foo?(bar) do
+        bar
+      else
+        false
+      end
+      """
+
+      code
+      |> run_task(UnnecessaryIfUnless, autocorrect: true)
+      |> assert_code(expected)
+    end
+
+    test "works with unless" do
+      code = """
+      unless foo?(bar) do
+        false
+      else
+        true
+      end
+      """
+
+      expected = """
+      foo?(bar)
+      """
+
+      code
+      |> run_task(UnnecessaryIfUnless, autocorrect: true)
+      |> assert_code(expected)
+    end
+
+    test "negates unless" do
+      code = """
+      unless foo?(bar) do
+        true
+      else
+        false
+      end
+      """
+
+      expected = """
+      not foo?(bar)
+      """
+
+      code
+      |> run_task(UnnecessaryIfUnless, autocorrect: true)
+      |> assert_code(expected)
+    end
+
+    test "reports no issues" do
+      """
+      foo == bar
+      """
+      |> run_task(UnnecessaryIfUnless, autocorrect: false)
+      |> refute_issues()
+    end
+
+    test "reports an issue" do
+      """
+      if foo == bar do
+        true
+      else
+        false
+      end
+      """
+      |> run_task(UnnecessaryIfUnless, autocorrect: false)
+      |> assert_issue_with(reporter: UnnecessaryIfUnless)
+    end
+  end
+end