Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added factory function check to MemberNameEqualsClassName #653

Merged
merged 4 commits into from Jan 1, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -8,10 +8,14 @@ import io.gitlab.arturbosch.detekt.api.Issue
import io.gitlab.arturbosch.detekt.api.Rule
import io.gitlab.arturbosch.detekt.api.Severity
import io.gitlab.arturbosch.detekt.rules.isOverridden
import org.jetbrains.kotlin.psi.KtBlockExpression
import org.jetbrains.kotlin.psi.KtClass
import org.jetbrains.kotlin.psi.KtClassBody
import org.jetbrains.kotlin.psi.KtClassOrObject
import org.jetbrains.kotlin.psi.KtNamedDeclaration
import org.jetbrains.kotlin.psi.KtNamedFunction
import org.jetbrains.kotlin.psi.KtObjectDeclaration
import org.jetbrains.kotlin.util.collectionUtils.concat

/**
*
Expand Down Expand Up @@ -47,37 +51,49 @@ class MemberNameEqualsClassName(config: Config = Config.empty) : Rule(config) {

override fun visitClass(klass: KtClass) {
if (!klass.isInterface()) {
checkClassOrObjectFunctions(klass, klass.name, classMessage)
checkCompanionObjectFunctions(klass)
getMisnamedMembers(klass, klass.name)
.concat(getMisnamedCompanionObjectMembers(klass))
?.forEach { report(CodeSmell(issue, Entity.from(it), classMessage)) }
}
super.visitClass(klass)
}

override fun visitObjectDeclaration(declaration: KtObjectDeclaration) {
if (!declaration.isCompanion()) {
checkClassOrObjectFunctions(declaration, declaration.name, objectMessage)
getMisnamedMembers(declaration, declaration.name)
.forEach { report(CodeSmell(issue, Entity.from(it), objectMessage)) }
}
super.visitObjectDeclaration(declaration)
}

private fun checkClassOrObjectFunctions(klassOrObject: KtClassOrObject, name: String?, message: String) {
val body = klassOrObject.getBody() ?: return
private fun getMisnamedMembers(klassOrObject: KtClassOrObject, name: String?): List<KtNamedDeclaration> {
val body = klassOrObject.getBody() ?: return emptyList()
val declarations = getFunctions(body).concat(body.properties)
return declarations?.filter { it.name?.equals(name, ignoreCase = true) == true } ?: emptyList()
}

private fun getFunctions(body: KtClassBody): List<KtNamedDeclaration> {
var functions = body.children.filterIsInstance<KtNamedFunction>()
if (ignoreOverriddenFunction) {
functions = functions.filter { !it.isOverridden() }
}
functions
.filter { it.name?.equals(name, ignoreCase = true) == true }
.forEach { report(CodeSmell(issue, Entity.from(it), message)) }
body.properties
.filter { it.name?.equals(name, ignoreCase = true) == true }
.forEach { report(CodeSmell(issue, Entity.from(it), message)) }
return functions
}

private fun checkCompanionObjectFunctions(klass: KtClass) {
klass.companionObjects.forEach {
checkClassOrObjectFunctions(it, klass.name, classMessage)
}
private fun getMisnamedCompanionObjectMembers(klass: KtClass): List<KtNamedDeclaration> {
val list = mutableListOf<KtNamedDeclaration>()
klass.companionObjects.forEach { list.addAll(getMisnamedMembers(it, klass.name)) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be just a flatMap-Funktion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

list
.filterIsInstance<KtNamedFunction>()
.filter { isFactoryMethod(it, klass) }
.forEach { it -> list.remove(it) }
return list
}

private fun isFactoryMethod(function: KtNamedFunction, klass: KtClass): Boolean {
val typeReference = function.typeReference
return typeReference == null && function.bodyExpression !is KtBlockExpression
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ignores functions which return a single expression since detekt cannot infer the type (at the moment).
Consider the following code for example

open class A {
  companion object {
    // return type = A
    fun a(condition: Boolean) = if (condition) B() else C()
  }
}

class B: A()
class C: A()

|| typeReference?.text?.equals(klass.name) == true
}

companion object {
Expand Down
Expand Up @@ -26,7 +26,7 @@ class MemberNameEqualsClassNameSpec : SubjectSpek<MemberNameEqualsClassName>({
val findings = subject.lint(path)

it("reports methods which are named after the class") {
assertThat(findings).hasSize(6)
assertThat(findings).hasSize(8)
}

it("reports methods which are named after the class object") {
Expand All @@ -37,7 +37,7 @@ class MemberNameEqualsClassNameSpec : SubjectSpek<MemberNameEqualsClassName>({
it("reports methods which are named after the class object including overridden functions") {
val config = TestConfig(mapOf("ignoreOverriddenFunction" to "false"))
val rule = MemberNameEqualsClassName(config)
assertThat(rule.lint(path)).hasSize(7)
assertThat(rule.lint(path)).hasSize(9)
}
}
})
Expand Up @@ -25,6 +25,24 @@ class StaticMethodNameEqualsObjectName {
}
}

// factory method can have the same name as the class
class FactoryClass1 {

companion object {
fun factoryClass1(): FactoryClass1 {
return FactoryClass1()
}
}
}

// factory method can have the same name as the class
class FactoryClass2 {

companion object {
fun factoryClass2() = FactoryClass2()
}
}

abstract class BaseClassForMethodNameEqualsClassName {

abstract fun AbstractMethodNameEqualsClassName()
Expand Down
Expand Up @@ -37,6 +37,22 @@ class MethodNameContainer {
}
}

class WrongFactoryClass1 {

companion object {
fun wrongFactoryClass1() {} // reports 1 - no return type
}
}

class WrongFactoryClass2 {

companion object {
fun wrongFactoryClass2(): Int { // reports 1 - wrong return type
return 0
}
}
}

class AbstractMethodNameEqualsClassName : BaseClassForMethodNameEqualsClassName() {

// reports if overridden functions are not ignored
Expand Down