Skip to content

Commit

Permalink
Find maximum length increasing section
Browse files Browse the repository at this point in the history
And based on that find the minimal violation
Original implementation comment at detekt#3142 (comment) which suggested the [algo](https://www.geeksforgeeks.org/minimum-insertions-sort-array/) from which this change is derived
  • Loading branch information
atulgpt committed Apr 15, 2023
1 parent d754502 commit d511575
Show file tree
Hide file tree
Showing 3 changed files with 106 additions and 45 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,12 @@ class FullQualifiedNameGuesser internal constructor(
imports: List<KtImportDirective>,
) {

@Suppress("ClassOrdering")
constructor(root: KtFile) : this(
packageName = root.packageDirective?.qualifiedName?.ifBlank { null },
imports = root.importList?.imports.orEmpty(),
)

@Suppress("ClassOrdering")
private val resolvedNames: Map<String, String> by lazy(NONE) {
imports
.asSequence()
Expand All @@ -28,7 +28,6 @@ class FullQualifiedNameGuesser internal constructor(
.toMap()
}

@Suppress("ClassOrdering")
private val starImports: List<String> by lazy(NONE) {
imports
.asSequence()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import org.jetbrains.kotlin.psi.KtObjectDeclaration
import org.jetbrains.kotlin.psi.KtProperty
import org.jetbrains.kotlin.psi.KtSecondaryConstructor

private typealias DeclarationToSectionPair = Pair<KtDeclaration, Section>

/**
* This rule ensures class contents are ordered as follows as recommended by the Kotlin
* [Coding Conventions](https://kotlinlang.org/docs/coding-conventions.html#class-layout):
Expand Down Expand Up @@ -64,60 +66,67 @@ class ClassOrdering(config: Config = Config.empty) : Rule(config) {
override fun visitClassBody(classBody: KtClassBody) {
super.visitClassBody(classBody)

var currentSection = Section(0)
val sectionList = classBody.declarations.filterNotNull()
if (sectionList.isEmpty()) return
getMinimalNumberOfViolations(sectionList).forEach { (f, s) ->
val (violationDeclarationToSectionPair, listOfIncreasingSection) = getMinimalNumberOfViolations(sectionList)
?: return
violationDeclarationToSectionPair.forEach { (declaration, section) ->
val (directionMsg, anchorSection) = listOfIncreasingSection.find { it.priority > section.priority }?.let {
"before" to it
} ?: run {
"after" to listOfIncreasingSection.findLast { it.priority < section.priority }
}
anchorSection ?: return@forEach
val message =
"${f.toDescription()} should be declared before ${s.toDescription()}."
"${declaration.toDescription()} should be declared $directionMsg ${anchorSection.toDescription()}."
report(
CodeSmell(
issue = issue,
entity = Entity.from(f),
entity = Entity.from(declaration),
message = message,
references = listOf(Entity.from(classBody))
)
)
}
for (ktDeclaration in classBody.declarations) {
val section = ktDeclaration.toSection() ?: continue
when {
section < currentSection -> {
val message =
"${ktDeclaration.toDescription()} should be declared before ${currentSection.toDescription()}."
report(
CodeSmell(
issue = issue,
entity = Entity.from(ktDeclaration),
message = message,
references = listOf(Entity.from(classBody))
)
)
}
section > currentSection -> currentSection = section
}
}
}

private fun getMinimalNumberOfViolations(declarations: List<KtDeclaration>): List<Pair<KtDeclaration, KtDeclaration>> {
val declarationWithSectionList = declarations.map {
it to it.toSection()
}.filter { it.second != null }
val dfs = IntArray(declarationWithSectionList.size) {
private fun getMinimalNumberOfViolations(
declarations: List<KtDeclaration>,
): Pair<List<DeclarationToSectionPair>, List<Section>>? {
val declarationWithSectionList = declarations.mapNotNull { declaration ->
declaration.toSection()?.let {
declaration to it
}
}
val dp = IntArray(declarationWithSectionList.size) {
return@IntArray 1
}
for (i in 1 until dfs.size) {
val backTrack = IntArray(declarationWithSectionList.size) {
return@IntArray it
}
for (i in 1 until dp.size) {
for (j in 0 until i) {
if (declarationWithSectionList[i].second!!.priority >= declarationWithSectionList[j].second!!.priority &&
dfs[i] < dfs[j] + 1
if (declarationWithSectionList[i].second.priority >= declarationWithSectionList[j].second.priority &&
dp[i] < dp[j] + 1
) {
dfs[i] = dfs[j] + 1
dp[i] = dp[j] + 1
backTrack[i] = j
}
}
}

val indexOfMax = dfs.indices.maxByOrNull { dfs[it] } ?: return emptyList()
var index = dp.indices.maxByOrNull { dp[it] } ?: return null

val listOfIncreasingSection = buildList {
var oldIndex: Int
do {
add(declarationWithSectionList[index])
oldIndex = index
index = backTrack[index]
} while (index != oldIndex)
}.reversed()
return declarationWithSectionList.minus(listOfIncreasingSection.toSet()) to
listOfIncreasingSection.map { it.second }
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,39 @@ class ClassOrderingSpec {
assertThat(subject.compileAndLint(code)).isEmpty()
}

@Test
fun `does not report when class is empty with empty body`() {
val code = """
class InOrder {
val y1 = 1
val y2 = 2
val y3 = 3
}
""".trimIndent()

assertThat(subject.compileAndLint(code)).isEmpty()
}

@Test
fun `does not report when class is empty with out body`() {
val code = """
class InOrder
""".trimIndent()

assertThat(subject.compileAndLint(code)).isEmpty()
}

@Test
fun `does not report when class has some order multiple properties`() {
val code = """
class InOrder {
}
""".trimIndent()

assertThat(subject.compileAndLint(code)).isEmpty()
}

@Test
fun `reports when class initializer block is out of order`() {
val code = """
Expand Down Expand Up @@ -102,12 +135,9 @@ class ClassOrderingSpec {
""".trimIndent()

val findings = subject.compileAndLint(code)
assertThat(findings).hasSize(2)
assertThat(findings).hasSize(1)
assertThat(findings[0].message).isEqualTo(
"property `y` should be declared before secondary constructors."
)
assertThat(findings[1].message).isEqualTo(
"initializer blocks should be declared before secondary constructors."
"secondary constructor should be declared before method declarations."
)
}

Expand All @@ -132,13 +162,9 @@ class ClassOrderingSpec {
""".trimIndent()

val findings = subject.compileAndLint(code)
assertThat(findings).hasSize(3)
assertThat(findings).hasSize(1)
assertThat(findings[0].message)
.isEqualTo("property `y` should be declared before method declarations.")
assertThat(findings[1].message)
.isEqualTo("initializer blocks should be declared before method declarations.")
assertThat(findings[2].message)
.isEqualTo("secondary constructor should be declared before method declarations.")
.isEqualTo("method `returnX()` should be declared before companion object.")
}

@Test
Expand Down Expand Up @@ -269,4 +295,31 @@ class ClassOrderingSpec {
assertThat(findings[2].message)
.isEqualTo("property `y` should be declared before companion object.")
}

@Test
fun `does report before issue when loweset sections comes after longest change`() {
val code = """
class SingleMisorderAtFirst(private val x: String) {
companion object {
const val IMPORTANT_VALUE = 3
}
val y = x
init {
println(y)
}
constructor(z: Int): this(z.toString())
fun returnX() = x
}
""".trimIndent()

val findings = subject.compileAndLint(code)
assertThat(findings).hasSize(1)
assertThat(findings[0].message)
.isEqualTo("companion object should be declared after method declarations.")
}
}

0 comments on commit d511575

Please sign in to comment.