From 32fcbebbd79b88af6d98ebd1a11ad06c5b467041 Mon Sep 17 00:00:00 2001
From: Fred Tingaud
 <95592999+frederic-tingaud-sonarsource@users.noreply.github.com>
Date: Thu, 21 Sep 2023 14:53:16 +0200
Subject: [PATCH] Modify rule S1871: LaYC format

---
 rules/S1871/rule.adoc | 73 ++++++++++++++++++++++++++++++-------------
 1 file changed, 52 insertions(+), 21 deletions(-)

diff --git a/rules/S1871/rule.adoc b/rules/S1871/rule.adoc
index 854a30af0f..5440921e14 100644
--- a/rules/S1871/rule.adoc
+++ b/rules/S1871/rule.adoc
@@ -1,27 +1,28 @@
 == Why is this an issue?
 
-include::description.adoc[]
+Having two `cases` in a `switch` statement or two branches in an `if` chain with the same implementation is at best duplicate code, and at worst a coding error.
 
-=== Noncompliant code example
-
-[source,text]
+[source,java,diff-id=1,diff-type=noncompliant]
 ----
 switch (i) {
-  case 1: 
+  case 1:
     doFirstThing();
     doSomething();
     break;
-  case 2: 
+  case 2:
     doSomethingDifferent();
     break;
   case 3:  // Noncompliant; duplicates case 1's implementation
     doFirstThing();
-    doSomething(); 
+    doSomething();
     break;
-  default: 
+  default:
     doTheRest();
 }
+----
 
+[source,java,diff-id=2,diff-type=noncompliant]
+----
 if (a >= 0 && a < 10) {
   doFirstThing();
   doTheThing();
@@ -34,17 +35,53 @@ else if (a >= 20 && a < 50) {
   doTheThing();  // Noncompliant; duplicates first condition
 }
 else {
-  doTheRest(); 
+  doTheRest();
 }
 ----
 
+If the same logic is truly needed for both instances, then:
+
+* in an `if` chain they should be combined
+
+[source,java,diff-id=2,diff-type=compliant]
+----
+if ((a >= 0 && a < 10) || (a >= 20 && a < 50)) { // Compliant
+  doFirstThing();
+  doTheThing();
+}
+else if (a >= 10 && a < 20) {
+  doTheOtherThing();
+}
+else {
+  doTheRest();
+}
+----
+
+* for a `switch`, one should fall through to the other.
+
+[source,java,diff-id=1,diff-type=compliant]
+----
+switch (i) {
+  case 1:
+  case 3: // Compliant
+    doFirstThing();
+    doSomething();
+    break;
+  case 2:
+    doSomethingDifferent();
+    break;
+  default:
+    doTheRest();
+}
+----
+
+When all blocks are identical, either this rule will trigger if there is no default clause or rule S3923 will raise if there is a default clause.
 
 === Exceptions
 
-Blocks in an ``++if++`` chain that contain a single line of code are ignored, as are blocks in a ``++switch++`` statement that contain a single line of code with or without a following ``++break++``.
+Unless all blocks are identical, blocks in an `if` chain that contain a single line of code are ignored. The same applies to blocks in a `switch` statement that contains a single line of code with or without a following `break`.
 
-
-[source,text]
+[source,java]
 ----
 if (a == 1) {
   doSomething();  //no issue, usually this is done on purpose to increase the readability
@@ -55,14 +92,8 @@ if (a == 1) {
 }
 ----
 
-But this exception does not apply to ``++if++`` chains without ``++else++``-s, or to ``++switch++``-es without default clauses when all branches have the same single line of code. In case of ``++if++`` chains with ``++else++``-s, or of ``++switch++``-es with default clauses, rule S3923 raises a bug. 
+== Resources
 
-[source,text]
-----
-if (a == 1) {
-  doSomething();  //Noncompliant, this might have been done on purpose but probably not
-} else if (a == 2) {
-  doSomething();
-}
-----
+=== Related rules
 
+* S3923 - All branches in a conditional structure should not have exactly the same implementation