detekt
New rule: Forbidden return type
I went through existing rules and I haven't find existing rule. Sorry if I happened to miss it 🙏🏼
Expected Behavior of the rule
"style" rule, that prevents specific function / property return types. I want to prevent specific type, but only for code with public API.
Context
In my case, I want to prevent having public functions or properties, that have Pair return type. I think this kind of types make code harder to understand.
// Bad
fun query(): Pair<Int, String> = TODO()
val query: Pair<Int, String> = TODO()
// Good
data class QueryResult(
val id: Int,
val name: String
)
fun query(): QueryResult = TODO()
val query: QueryResult = TODO()
private fun somethingInternal(): Pair<Int, Int>
In my case, I want to prevent having public functions or properties, that have
Pairreturn type. I think this kind of types make code harder to understand.
I don't think that:
fun query(): Pair<Int, String>
is hard to read. Pair also have destructuring which makes it a great abstraction for functions that need to return 2 values.
Can you provide another example of a type you don't want to be used as return type 🤔
What I don't like about Pair is that it has unspecific property names (first and second), which don't help with understanding the code. And I think that dealing with multiple pairs at the same time can make it even more confusing.
Creating a data class with more meaningful property names is super simple anyway.
Yeah, you can use destructuring for specific naming (e.g. from OP val (id, name) = query()), but I think having a variable with concrete name is more robust for refactoring. And let's face it, if you have first and second someone will use it at some point.
There are articles written on this. Google Guava even describes them as "Tuple types are awful obfuscators".
Other use cases that come to my mind:
-
Triplefor same reasons asPair - Any other generic type could maybe be blacklisted. For example something as
Map<*, *>orList<*>.
This issue is stale because it has been open 90 days with no activity. Please comment or this will be closed in 7 days.
This issue is stale because it has been open 90 days with no activity. Please comment or this will be closed in 7 days.
I have another case: kotlin.Result. I think that this is not the first rule where I talk about this, but I can't recall where.
But what I'm not sure about is "on public functions". Basically for simetry with our other forbidden rules.
In general I like the "forbidden" rules. They give a lot of flexibility to the teams with really little investment.
TL;DR: I would approve a pr with rule like this. But the "only on public" should be a configuration flag disabled by default.
"only on public" is for sure very debatable, maybe it deserves separate issue/discussion, so I don't mind if you don't like that idea.
What I had in my mind is to be less restrictive, when it's not that important. I think public API should be more understandable and architectually "stable" for consumers than private API. If you're using a library, you often only care about public API.
If you for example have private fun x(): Pair, it doesn't need to have same restrictions in place as public fun y(): Pair. Private x() can be used just a few times, but it's more likely that public y() could be used all over the place. Changing type from something like Pair<Int, Int> to Triple<Int, String, Int> can be more annoying that having something like QueryResult.
Indeed. I understand your point. I like the idea about adding the rule without the flag and create another issue for that. And that's because we could create a suppressor for that so it can be used on any rule.
