ClassOrdering reports a message describing the misorder (#3138)

* ClassOrdering reports a message describing the misorder

* remove empty line

* tests for nested class and anonymous object

* tests for message

* Fixes description strings for class initializer and companion object

detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ClassOrdering.kt

@@ -60,33 +60,53 @@ class ClassOrdering(config: Config = Config.empty) : Rule(config) {
         Debt.FIVE_MINS
     )
 
-    private val lengthComparator: Comparator<KtDeclaration> = Comparator { str1: KtDeclaration, str2: KtDeclaration ->
-        if (orderPriority(str1) == null || orderPriority(str2) == null) return@Comparator 0
-        compareValues(orderPriority(str1), orderPriority(str2))
+    private val comparator: Comparator<KtDeclaration> = Comparator { dec1: KtDeclaration, dec2: KtDeclaration ->
+        if (dec1.priority == null || dec2.priority == null) return@Comparator 0
+        compareValues(dec1.priority, dec2.priority)
     }
 
     override fun visitClassBody(classBody: KtClassBody) {
         super.visitClassBody(classBody)
 
-        if (!classBody.declarations.isInOrder(lengthComparator)) {
-            report(CodeSmell(issue, Entity.from(classBody), issue.description))
+        comparator.findOutOfOrder(classBody.declarations)?.let {
+            report(CodeSmell(issue, Entity.from(classBody), generateMessage(it)))
         }
     }
 
-    @Suppress("MagicNumber")
-    private fun orderPriority(declaration: KtDeclaration): Int? {
-        return when (declaration) {
-            is KtProperty -> 0
-            is KtClassInitializer -> 0
-            is KtSecondaryConstructor -> 1
-            is KtNamedFunction -> 2
-            is KtObjectDeclaration -> if (declaration.isCompanion()) 3 else null
-            else -> null
-        }
+    private fun generateMessage(misordered: Pair<KtDeclaration, KtDeclaration>): String {
+        return "${misordered.first.description} should not come before ${misordered.second.description}"
     }
+}
 
-    private fun Iterable<KtDeclaration>.isInOrder(comparator: Comparator<KtDeclaration>): Boolean {
-        zipWithNext { a, b -> if (comparator.compare(a, b) > 0) return false }
-        return true
-    }
+private fun Comparator<KtDeclaration>.findOutOfOrder(
+    declarations: List<KtDeclaration>
+): Pair<KtDeclaration, KtDeclaration>? {
+    declarations.zipWithNext { a, b -> if (compare(a, b) > 0) return Pair(a, b) }
+    return null
 }
+
+private val KtDeclaration.description: String
+    get() = when (this) {
+        is KtClassInitializer -> "class initializer"
+        is KtObjectDeclaration -> if (isCompanion()) "Companion object" else ""
+        else -> "$name ($printableDeclaration)"
+    }
+
+private val KtDeclaration.printableDeclaration: String
+    get() = when (this) {
+        is KtProperty -> "property"
+        is KtSecondaryConstructor -> "secondary constructor"
+        is KtNamedFunction -> "function"
+        else -> ""
+    }
+
+@Suppress("MagicNumber")
+private val KtDeclaration.priority: Int?
+    get() = when (this) {
+        is KtProperty -> 0
+        is KtClassInitializer -> 0
+        is KtSecondaryConstructor -> 1
+        is KtNamedFunction -> 2
+        is KtObjectDeclaration -> if (isCompanion()) 3 else null
+        else -> null
+    }

detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ClassOrderingSpec.kt

@@ -74,7 +74,10 @@ class ClassOrderingSpec : Spek({
                 }
             """.trimIndent()
 
-            assertThat(subject.compileAndLint(code)).hasSize(1)
+            val findings = subject.compileAndLint(code)
+            assertThat(findings).hasSize(1)
+            assertThat(findings[0].message).isEqualTo("OutOfOrder (secondary constructor) " +
+                "should not come before class initializer")
         }
 
         it("reports when secondary constructor is out of order") {
@@ -96,7 +99,10 @@ class ClassOrderingSpec : Spek({
                 }
             """.trimIndent()
 
-            assertThat(subject.compileAndLint(code)).hasSize(1)
+            val findings = subject.compileAndLint(code)
+            assertThat(findings).hasSize(1)
+            assertThat(findings[0].message).isEqualTo("OutOfOrder (secondary constructor) " +
+                "should not come before y (property)")
         }
 
         it("reports when method is out of order") {
@@ -118,7 +124,9 @@ class ClassOrderingSpec : Spek({
                 }
             """.trimIndent()
 
-            assertThat(subject.compileAndLint(code)).hasSize(1)
+            val findings = subject.compileAndLint(code)
+            assertThat(findings).hasSize(1)
+            assertThat(findings[0].message).isEqualTo("returnX (function) should not come before y (property)")
         }
 
         it("reports when companion object is out of order") {
@@ -140,7 +148,53 @@ class ClassOrderingSpec : Spek({
                 }
             """.trimIndent()
 
-            assertThat(subject.compileAndLint(code)).hasSize(1)
+            val findings = subject.compileAndLint(code)
+            assertThat(findings).hasSize(1)
+            assertThat(findings[0].message).isEqualTo("Companion object should not come before returnX (function)")
+        }
+
+        it("does not report nested class order") {
+            val code = """
+                class OutOfOrder(private val x: String) {
+                    val y = x
+
+                    init {
+                        check(x == "yes")
+                    }
+
+                    constructor(z: Int): this(z.toString())
+
+                    class Nested {
+                        fun foo() = 2
+                    }
+
+                    fun returnX() = x
+                }
+            """.trimIndent()
+
+            assertThat(subject.compileAndLint(code)).hasSize(0)
+        }
+
+        it("does not report anonymous object order") {
+            val code = """
+                class OutOfOrder(private val x: String) {
+                    val y = x
+
+                    init {
+                        check(x == "yes")
+                    }
+
+                    constructor(z: Int): this(z.toString())
+
+                    object AnonymousObject {
+                        fun foo() = 2
+                    }
+
+                    fun returnX() = x
+                }
+            """.trimIndent()
+
+            assertThat(subject.compileAndLint(code)).hasSize(0)
         }
     }
 })