Kotlin code style consistency with other codebases

[tomholub]

@DenBond7 this is an interesting thing - after a lot of discussion between me, Ivan and Julian on other repositories, we settled for the pattern that Ivan used above, and we disabled the IDE nudges to refactor it as you suggest.

The reason is that we'd like the code to be very understandable to people who only know Java and see Kotlin for the first time (which may be the case for the security reviewers for example, or for new team members).

And so instead of:

val x = if(cond) {
  1
} else {
  2
}

we'd write

val x: Int
if(cond) {
  x = 1
} else {
  x = 2
}

Similarly with try/catch. It's more code but we think it increases clarity.

In situations when you really want succinct usage, we can use when which is I think better structured for implicit returns:

val x = when (cond) {
  true -> x,
  false -> y
}

Generally also trying to avoid implicit returns in long lambdas, like this:

whatever(something, lambda = {
  something()
  else()
- thisIsReturnedButItsNotObvious()
+ return@lambda thisIsClearer()
})

We have never discussed code style much on Android because you were the only contributor and the code style was already sensible. However as we have several people who may contribute to Kotlin code these days, we should use relatively consistent styling across repositories.

(I'll file an issue with this comment)

[DenBond7]

understand. Now I see the reason. I agree with that style.

we disabled the IDE nudges to refactor it as you suggest.

@IvanPizhenko Could you look if we can add it as repo settings?

[IvanPizhenko]

yes, I think we can do that. I'll do that after finishing some other things like decryption.

[tomholub]

copying more comments here so that style is in one place. We should probably write a document like STYLE.md

---

if/else statements do not have to have braces if on one line. But in that case it's only permitted to use for "consistent" result, eg like a ternary statement return if (c) 1 else 2 or val x = if (c) 1 else 2. Cannot mix return and throw on one line or without braces.

if/else statements MUST have braces if multiline. I wish there was some ktlint rule for this (maybe there is?)

// good
val x = if(c) 1 else 2

// good
return if(c) 1 else 2

// bad
if (key is PGPSecretKeyRing)
      return key
    else
     throw IllegalArgumentException("Key is not a secret key")

// good
if (key is PGPSecretKeyRing) {
  return key
} else {
  throw IllegalArgumentException("Key is not a secret key")
}

// also good but depends on style? I prefer this but other people prefer the above. Not much difference
if (key is PGPSecretKeyRing) {
  return key
}
throw IllegalArgumentException("Key is not a secret key")

This is also good. Maybe the best - I find it succinct and clear:

// good
return (key as? PGPSecretKeyRing) ?: throw IllegalArgumentException("Key is not a secret key")

// if it doesn't fit on one line, a `?: throw` on next line is also good
return (key as? PGPSecretKeyRing)
  ?: throw IllegalArgumentException("Key is not a secret key")

I think everybody will have a little different idea, but I think this is a decent compromise.

[tomholub]

Other random thoughts:

• ban StringBuilder except in performance-sensitive usecases (it's over-optimization to use it for 10 small strings, + is more readable)
• we have no issue with singletons (object Something { })
• ban Pair and Triple, make a data class instead
• enums preferred over strings when it makes sense

[DenBond7]

#584 #545

[DenBond7]

#352

[IvanPizhenko]

That's all fragmented across multiple issues. I'd suggest to put everything into single markdown doc and add it into repo.

[DenBond7]

EOF(end of file)