Kotlin / kotlin-style-guide

Work-in-progress notes for the Kotlin style guide
288 stars 14 forks source link

Accessing properties which are open, var, or do not have backing fields #40

Open damianw opened 7 years ago

damianw commented 7 years ago

When accessing a property multiple times which cannot be smart cast, i.e. is any of the following:

Prefer to first assign the property to a local variable.

val foo: String
    get() { /* ... */ }

fun good() {
  val foo = foo
  action1(foo)
  action2(foo)
}

fun bad() {
  action1(foo)
  action2(foo)
}
cbruegg commented 7 years ago

I think this is too broad. I'd say do this only if you actually need to (smart-)cast, but that's kind of obvious anyway so there would be no need for a dedicated rule in the style guide in my opinion.

Damian Wieczorek notifications@github.com schrieb am Mi., 31. Mai 2017, 17:58:

When accessing a property multiple times which cannot be smart cast, i.e. is any of the following:

  • open
  • a var
  • does not have a backing field

Prefer to first assign the property to a local variable.

val foo: String get() { / ... / } fun good() { val foo = foo action1(foo) action2(foo) } fun bad() { action1(foo) action2(foo) }

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/yole/kotlin-style-guide/issues/40, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKtPQtWLuFoA2DbB6rtmwZ3kiMDkAY5ks5r_Y4KgaJpZM4Nr15W .

damianw commented 7 years ago

@cbruegg That's for sure; I had a similar feeling. I've phrased it as "prefer to first..." for that reason. While not strictly necessary in every situation, I think that repeated access of such a property is indicative of code smell. Kotlin properties provide a false sense of simplicity because of how easy they are to access. The equivalent Java code would be calling getFoo several times over, which should be discouraged, given that so many APIs return defensive copies, compute values on-demand, etc.

Supuhstar commented 7 years ago

I like this; it encourages the very good practice of caching computed values, which generally speeds up an app

benjishults commented 7 years ago

Why not use foo.let { action1(it); action2(it) }?