Repository: nicoespeon/abracadabra
Pull request #23 (2019-08-16 00:22:03)

Flip If/Else now handle guard clauses
Consider this guard clause example:

```js
function doSomething(someData) {
  if (!isValid(someData)) {
    return;
  }

  // … rest of the code
}
```

Before, running _Flip If/Else_ would have produced:

```js
function doSomething(someData) {
  if (isValid(someData)) {
  } else {
    return;
  }

  // … rest of the code
}
```

Which is valid, but probably not what had in mind.

Now, it would produce the following result:

```js
function doSomething(someData) {
  if (isValid(someData)) {
    // … rest of the code
  }
}
```


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 50c07229..b895f383 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,6 +7,47 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed
+
+- Flip If/Else now works better on [guard clause][guard-clause] patterns
+
+Consider this guard clause example:
+
+```js
+function doSomething(someData) {
+  if (!isValid(someData)) {
+    return;
+  }
+
+  // … rest of the code
+}
+```
+
+Before, running _Flip If/Else_ would have produced:
+
+```js
+function doSomething(someData) {
+  if (isValid(someData)) {
+  } else {
+    return;
+  }
+
+  // … rest of the code
+}
+```
+
+Which is valid, but probably not what had in mind.
+
+Now, it would produce the following result:
+
+```js
+function doSomething(someData) {
+  if (isValid(someData)) {
+    // … rest of the code
+  }
+}
+```
+
 ### Fixed
 
 - Extract Variable on JSX Elements now triggers symbol rename as expected
@@ -68,3 +109,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 [0.2.0]: https://github.com/nicoespeon/abracadabra/compare/0.1.0...0.2.0
 [0.1.0]: https://github.com/nicoespeon/abracadabra/compare/0.0.1...0.1.0
 [0.0.1]: https://github.com/nicoespeon/abracadabra/compare/224558fafc2c9247b637a74a7f17fe3c62140d47...0.0.1
+
+<!-- Links -->
+
+[guard-clause]: https://deviq.com/guard-clause/
diff --git a/src/array-helpers.ts b/src/array-helpers.ts
new file mode 100644
index 00000000..90e8c54b
--- /dev/null
+++ b/src/array-helpers.ts
@@ -0,0 +1,9 @@
+export { last, allButLast };
+
+function last<T>(array: T[] | ReadonlyArray<T>): T {
+  return array[array.length - 1];
+}
+
+function allButLast<T>(array: T[]): T[] {
+  return array.slice(0, -1);
+}
diff --git a/src/ast/domain.ts b/src/ast/domain.ts
index 148f5b93..eb8563ab 100644
--- a/src/ast/domain.ts
+++ b/src/ast/domain.ts
@@ -1,10 +1,16 @@
+import { NodePath } from "@babel/traverse";
 import * as t from "@babel/types";
 
+import { last } from "../array-helpers";
+
 export * from "@babel/types";
 export {
   isArrayExpressionElement,
   areAllObjectProperties,
   isUndefinedLiteral,
+  isGuardClause,
+  isGuardConsequentBlock,
+  isNonEmptyReturn,
   isTemplateExpression,
   templateElement,
   Primitive
@@ -29,6 +35,23 @@ function isUndefinedLiteral(
   return t.isIdentifier(node, opts) && node.name === "undefined";
 }
 
+function isGuardClause(path: NodePath<t.IfStatement>) {
+  const { consequent } = path.node;
+  return t.isReturnStatement(consequent) || isGuardConsequentBlock(consequent);
+}
+
+function isGuardConsequentBlock(
+  consequent: t.IfStatement["consequent"]
+): consequent is t.BlockStatement {
+  return (
+    t.isBlockStatement(consequent) && t.isReturnStatement(last(consequent.body))
+  );
+}
+
+function isNonEmptyReturn(node: t.Node) {
+  return t.isReturnStatement(node) && node.argument !== null;
+}
+
 function isTemplateExpression(node: t.Node): node is TemplateExpression {
   return (
     t.isIdentifier(node) ||
diff --git a/src/refactorings/flip-if-else/flip-if-else.ts b/src/refactorings/flip-if-else/flip-if-else.ts
index 1a942f6e..d0cf4019 100644
--- a/src/refactorings/flip-if-else/flip-if-else.ts
+++ b/src/refactorings/flip-if-else/flip-if-else.ts
@@ -1,6 +1,7 @@
 import { Editor, Code, ErrorReason } from "../../editor/editor";
 import { Selection } from "../../editor/selection";
 import * as ast from "../../ast";
+import { last, allButLast } from "../../array-helpers";
 
 import { getNegatedBinaryOperator } from "../negate-expression/negate-expression";
 
@@ -37,17 +38,60 @@ function updateCode(code: Code, selection: Selection): ast.Transformed {
       // if a child would match the selection closer.
       if (hasChildWhichMatchesSelection(path, selection)) return;
 
-      const ifBranch = node.consequent;
-      const elseBranch = node.alternate || ast.blockStatement([]);
-      node.consequent = ast.isIfStatement(elseBranch)
-        ? ast.blockStatement([elseBranch])
-        : elseBranch;
-      node.alternate = ifBranch;
+      if (ast.isGuardClause(path)) {
+        flipGuardClause(path);
+      } else {
+        flipIfStatement(path);
+      }
+
       node.test = getNegatedIfTest(node.test);
     }
   });
 }
 
+function flipIfStatement(path: ast.NodePath<ast.IfStatement>) {
+  const ifBranch = path.node.consequent;
+  const elseBranch = path.node.alternate || ast.blockStatement([]);
+
+  path.node.consequent = ast.isIfStatement(elseBranch)
+    ? ast.blockStatement([elseBranch])
+    : elseBranch;
+  path.node.alternate = ifBranch;
+}
+
+function flipGuardClause(path: ast.NodePath<ast.IfStatement>) {
+  const ifBranch = path.node.consequent;
+  const pathsBelow = path
+    .getAllNextSiblings()
+    .filter(
+      (path): path is ast.NodePath<ast.Statement> => ast.isStatement(path)
+    );
+  const nodesBelow: ast.Statement[] = pathsBelow.map(path => path.node);
+
+  path.node.consequent = ast.blockStatement(nodesBelow);
+  path.node.alternate = flipToGuardAlternate(ifBranch);
+  pathsBelow.forEach(path => path.remove());
+}
+
+function flipToGuardAlternate(
+  consequent: ast.Statement
+): ast.BlockStatement | null {
+  if (ast.isNonEmptyReturn(consequent)) {
+    return ast.blockStatement([consequent]);
+  }
+
+  if (!ast.isGuardConsequentBlock(consequent)) return null;
+
+  const finalReturnStatement = last(consequent.body);
+  const alternateBody = ast.isNonEmptyReturn(finalReturnStatement)
+    ? consequent.body
+    : allButLast(consequent.body);
+
+  if (alternateBody.length === 0) return null;
+
+  return ast.blockStatement(alternateBody);
+}
+
 function hasChildWhichMatchesSelection(
   path: ast.NodePath,
   selection: Selection
diff --git a/src/refactorings/inline-variable-or-function/inline-variable.ts b/src/refactorings/inline-variable-or-function/inline-variable.ts
index 8587a3f6..7d72b0c4 100644
--- a/src/refactorings/inline-variable-or-function/inline-variable.ts
+++ b/src/refactorings/inline-variable-or-function/inline-variable.ts
@@ -1,6 +1,7 @@
 import { Editor, Code, ErrorReason } from "../../editor/editor";
 import { Selection } from "../../editor/selection";
 import * as ast from "../../ast";
+import { last } from "../../array-helpers";
 
 import { findExportedIdNames } from "./find-exported-id-names";
 
@@ -161,7 +162,7 @@ function findIdentifiersToReplace(
       const isSameIdentifier = selection.isInsideNode(id);
       if (isSameIdentifier) return;
 
-      const parent = ancestors[ancestors.length - 1];
+      const parent = last(ancestors);
       if (ast.isFunctionDeclaration(parent)) return;
       if (ast.isObjectProperty(parent.node) && parent.node.key === node) return;
       if (
diff --git a/src/refactorings/remove-redundant-else/remove-redundant-else.ts b/src/refactorings/remove-redundant-else/remove-redundant-else.ts
index 977ad60d..ff364d07 100644
--- a/src/refactorings/remove-redundant-else/remove-redundant-else.ts
+++ b/src/refactorings/remove-redundant-else/remove-redundant-else.ts
@@ -1,6 +1,7 @@
 import { Editor, Code, ErrorReason } from "../../editor/editor";
 import { Selection } from "../../editor/selection";
 import * as ast from "../../ast";
+import { last } from "../../array-helpers";
 
 export { removeRedundantElse, hasRedundantElse };
 
@@ -81,7 +82,7 @@ function hasChildWhichMatchesSelection(
 }
 
 function hasExitStatement(node: ast.BlockStatement): boolean {
-  const lastStatement = node.body[node.body.length - 1];
+  const lastStatement = last(node.body);
 
   return (
     ast.isReturnStatement(lastStatement) || ast.isThrowStatement(lastStatement)


Test patch:
diff --git a/src/refactorings/flip-if-else/flip-if-else.test.ts b/src/refactorings/flip-if-else/flip-if-else.test.ts
index 745419f8..4603403e 100644
--- a/src/refactorings/flip-if-else/flip-if-else.test.ts
+++ b/src/refactorings/flip-if-else/flip-if-else.test.ts
@@ -127,6 +127,88 @@ describe("Flip If/Else", () => {
   }
 } else {
   doAnotherThing();
+}`
+      },
+      {
+        description: "guard clause",
+        code: `if (!isValid) return;
+
+doSomething();
+doSomethingElse();`,
+        expected: `if (isValid) {
+  doSomething();
+  doSomethingElse();
+}`
+      },
+      {
+        description: "guard clause with block statement",
+        code: `if (!isValid) {
+  return;
+}
+
+doSomething();
+doSomethingElse();`,
+        expected: `if (isValid) {
+  doSomething();
+  doSomethingElse();
+}`
+      },
+      {
+        description: "guard clause with other statements in block",
+        code: `if (!isValid) {
+  console.log("Hello");
+  return;
+}
+
+doSomething();
+doSomethingElse();`,
+        expected: `if (isValid) {
+  doSomething();
+  doSomethingElse();
+} else {
+  console.log("Hello");
+}`
+      },
+      {
+        description: "guard clause with other statements above",
+        code: `console.log("Hello");
+if (!isValid) return;
+
+doSomething();
+doSomethingElse();`,
+        selection: Selection.cursorAt(1, 16),
+        expected: `console.log("Hello");
+if (isValid) {
+  doSomething();
+  doSomethingElse();
+}`
+      },
+      {
+        description: "guard clause with returned value",
+        code: `if (!isValid) return null;
+
+doSomething();
+doSomethingElse();`,
+        expected: `if (isValid) {
+  doSomething();
+  doSomethingElse();
+} else {
+  return null;
+}`
+      },
+      {
+        description: "guard clause with returned value in block",
+        code: `if (!isValid) {
+  return null;
+}
+
+doSomething();
+doSomethingElse();`,
+        expected: `if (isValid) {
+  doSomething();
+  doSomethingElse();
+} else {
+  return null;
 }`
       }
     ],