Decide a code convention to follow

[fondesa]

We should decide a set of code convention rules to follow for:

• Kotlin
• Groovy
• Bash

After doing this, we should also remove the entry .idea from .gitignore, ignoring only the files which should not be shared (see here).

[fardavide]

For Kotlin I'd follow the default code-style, which is enforced by the rule-set defined in #8

Said rule-set enforces most of the conventions: I think we can start from there and adapt it to our needing.

I have a few preferences that are not enforced by this rule-set, but we could look into creating custom Detekt rules for that:

• Avoid using top-level constants:
  we could have cases where we cannot keep them private in the file, so we will be forced to:

  • rename them if they were private
  • adopt a long and complex name to give them a scope
  • clutter the namespace

  I would prefer to define them inside a companion object in a short meaningful name, using UpperCamelCase, e.g.: SomeRepository.MaxRefreshInterval, over SOMETHING_SOMETHING_MAX_REFRESH_INTERVAL

• Alphabetical order -almost- everywhere:
  There are cases where we can have a clear logical order (e.g. public functions above and private below, or other cases that I'm going to list one by one) and other where that is not so clear (e.g. MovieModel(id, title, credits, releaseDate)) in those cases, we can establish (or, better, force) some logic, but it would be troublesome to maintain, causing chaos and making it hard to understand where to add a new member or parameter. For all these cases, I'd be for keeping the alphabetical-order rule

• Avoid default parameters:
  I've seen people abusing default parameters (e.g. MovieUiModel(title = "", releaseYear = 0, actors = emptyList()) to facilitate the instance creation for various reasons. I find this extremely dangerous! We have an excellent compiler and we should leverage its power, allowing it to block our build if we add a new parameter in the declaration and we didn't declare it in the caller, instead of risking to have MovieUiModel.title = "" in our app. We can adopt various solutions, instead of having defaults, like having a great set of Samples, like MovieUiModelSample.Inception, to reuse for our tests or Compose's previews

[fondesa]

For Kotlin I'd follow the default code-style, which is enforced by the rule-set defined in #8

+1

Avoid using top-level constants

+1, they are evil.

Alphabetical order -almost- everywhere:

I wouldn't enforce this. I agree there are cases where it could make sense (e.g. order of fields in a data class constructor), but there are others where it would decrease the readability by a lot (e.g. having functions by relevance would be much better than alphabetical IMHO). As you said, we can't enforce these specific checks but PR reviews are a good place for this type of feedback. Another cons is related to PRs: if I rename a function, I'll probably need to change its order as well and this would worsen PR reviews (e.g. moving a code block would appear as the code block deletion + the code block addition instead of just 1 line change).

Avoid default parameters:

+1. Sometimes they can be useful but risk here is so much greater than the reward.

[fardavide]

Completely agree about functions, also for params it can be tricky, for example for @composable functions we can have a well defined pattern Composable(State/Model, Actions, Modifier) and it applies to 99% of the cases.

For this matter, we can observe the pattern and create some Detekt rules.

About functions, we could keel the alphabetical order in some cases, for example, in class in the data layer (Repository or Data Source), ordering alphabetically should meet the ordering by context, for example:

fun deleteMovie
fun deleteTvShow
fun favoriteMovie
fun favoriteTvShow
fun insertMovie
fun insertTvShow

[fondesa]

(moved this to discussions)

[mcatta]

I agree with the proposals, but maybe I would be a bit conservative about the following:

Alphabetical order -almost- everywhere:

given that I love consistency in the codebase, but at the same time, I would like fewer constraints where they are unnecessary. I would like to apply them in specific cases like:

• interfaces
• abstract classes
• data classes

If is it possible I would like to add a new rule (if not exists yet): can we forbid adding anonymous functions inside another function? Lie:

fun foo() {
   fun anonymousFoo(value: Int) {}

   anonymousFoo(2)
}

I have seen this approach in some projects but honestly, I think it makes the code less readable, but I would like to hear your opinion too.

[fondesa]

My opinion on nested functions changed over the years. At the beginning I was using them but I got multiple feedbacks on code being less readable (basically the same @mcatta was saying) so I changed my idea on them and don't use them anymore. Since that moment, I was able to reproduce the same exact behavior without using nested functions. So, I'd vote to forbid nested functions for the motivations above

[fardavide]

I'm speaking theoretically, if we can actually avoid them, without sacrificing other aspects, I'm up for it.

We can forbid them and then see if we have a real case to discuss

[fondesa]

I'm speaking theoretically as well. In fact I think we can also do the opposite: allow them and then see if we start to abuse them

[fardavide]

Nothing forbids us to write a nested function and suppress the warning and discuss it in core review 🙂

[fardavide]

ForbiddenLocalFunction rule added to the project