Refactor + rename util function inside MandatoryBracesIfStatement rule

[schalkms]

No description provided.

[schalkms]

Maybe you have better ideas for the function name.

[marschwar]

hasCorrectElseType is really wrong. It should be hasIncorrectElseType.

I played around with it a little bit and realized that we do allow a when expression within the else branch but not in the then branch. It may not be something that is used often, but I don't see a reason why this is compliant

fun f(i: Int) {
    if (true) {
        println()
    } else when(i) {
        1 -> println(1)
        else -> println()
    }
}

while this is not

fun f(i: Int) {
    if (true) when(i) {
        1 -> println(1)
        else -> println()
    } else when(i) {
        println()
    }
}

I ended up with

val thenExpression = expression.then
if (mustBeOnSameLine(thenExpression) && hasNewLineAfter(expression.rightParenthesis)) {
    report(CodeSmell(issue, Entity.from(thenExpression ?: expression), DESCRIPTION))
}

val elseExpression = expression.`else`
if (mustBeOnSameLine(elseExpression) && hasNewLineAfter(expression.elseKeyword)) {
    report(CodeSmell(issue, Entity.from(elseExpression ?: expression), DESCRIPTION))
}

private fun mustBeOnSameLine(expression: KtExpression?): Boolean {
    return expression != null &&
        expression !is KtIfExpression &&
        expression !is KtBlockExpression &&
        expression !is KtWhenExpression
}

[schalkms]

Thanks for the suggestion. I have the feeling that experimental contracts are a bit overkill in this case. I'm also not sure whether we want experimental features being used in detekt's code base.

[marschwar]

I totally agree. I updated the comment above removing the contract.

[codecov]

Codecov Report

Merging #3908 (83f90ad) into main (44c1ef0) will increase coverage by 0.03%.
The diff coverage is 84.61%.

@@             Coverage Diff              @@
##               main    #3908      +/-   ##
============================================
+ Coverage     83.53%   83.56%   +0.03%     
+ Complexity     3120     3118       -2     
============================================
  Files           456      456              
  Lines          8996     8995       -1     
  Branches       1753     1750       -3     
============================================
+ Hits           7515     7517       +2     
  Misses          565      565              
+ Partials        916      913       -3     

Impacted Files	Coverage Δ
...ules/style/optional/MandatoryBracesIfStatements.kt	86.66% <84.61%> (+17.91%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44c1ef0...83f90ad. Read the comment docs.

[schalkms]

By doing the null check before, we can safely pass the else variable here.