Language Abuse & Unreadability 

No content

"Can you read my code?" by Ivan Morgillo Video : https://goo.gl/5fbd1d Presentation: https://goo.gl/ZwRiNF 

Conventions • Coding Conventions by Kotlin https://kotlinlang.org/docs/reference/coding- conventions.html • Android Kotlin Guides https://android.github.io/kotlin-guides/style.html • Detekt - Static code analysis https://github.com/arturbosch/detekt • An anti-bikeshedding Kotlin linter https://ktlint.github.io/ 

No Package Private private is file visible, internal is module visible 

Options • Put every class into one file • How readable is it? • How easy to discover them? Extract into module and use • internal instead of private Reconsider the architecture, implementation. • 

Standard Functions Let - Apply - Also - Run - With 

/** * Calls the specified function [block] with `this` value as its argument and returns its result. */ @kotlin.internal.InlineOnly public inline fun T.let(block: (T) -> R): R = block(this) /** * Calls the specified function [block] and returns its result. */ @kotlin.internal.InlineOnly public inline fun run(block: () -> R): R = block() /** * Calls the specified function [block] with `this` value as its receiver and returns its result. */ @kotlin.internal.InlineOnly public inline fun T.run(block: T.() -> R): R = block() /** * Calls the specified function [block] with the given [receiver] as its receiver and returns its result. */ @kotlin.internal.InlineOnly public inline fun with(receiver: T, block: T.() -> R): R = receiver.block() /** * Calls the specified function [block] with `this` value as its receiver and returns `this` value. */ @kotlin.internal.InlineOnly public inline fun T.apply(block: T.() -> Unit): T { block(); return this } /** * Calls the specified function [block] with `this` value as its argument and returns `this` value. */ @kotlin.internal.InlineOnly @SinceKotlin("1.1") public inline fun T.also(block: (T) -> Unit): T { block(this); return this } 

override fun onCreateView( inflater: LayoutInflater, container: ViewGroup?, savedInstanceState: Bundle? ): View? { return inflater.inflate(R.layout.some_layout, container, false).apply { recyclerView = findViewById(R.id.recycler_view) recyclerView.apply { setHasFixedSize(false) layoutManager = LinearLayoutManager(context) } } } Irrelevant usage 

override fun onCreateView( inflater: LayoutInflater, container: ViewGroup?, savedInstanceState: Bundle? ): View? { return inflater.inflate(R.layout.some_layout, container, false).apply { findViewById(R.id.recycler_view).apply { setHasFixedSize(false) layoutManager = LinearLayoutManager(context) } } } Irrelevant usage 

Unnecessary Usage fun getCardHeight(cardId: String): Int { (cardPositionMap[cardId] ?: 0) .let { cardIndex -> val componentListLength = (cardComponentListMap[cardId]?.size ?: 0) return (cardIndex until cardIndex + componentListLength).sumBy { processedHeightList[it] } } } 

Unnecessary Usage fun getCardHeight(cardId: String): Int { val cardIndex = cardPositionMap[cardId] ?: 0) val componentListLength = (cardComponentListMap[cardId]?.size ?: 0) return (cardIndex until cardIndex + componentListLength).sumBy { processedHeightList[it] } } 

Unnecessary Usage repoViewModel.getRepositories(pageNumber, shouldFetchFromServer) ... .subscribe( { result -> result?.forEach { item -> run { mAdapter.add(repoViewModel.transformToRepoItem(item)) mAdapter.notifyAdapterDataSetChanged() } } }, { error -> ... }) runOnUiThread 

Chaining fun RendererAdapter>.markAsViewed(recyclerView: RecyclerView, id: CardId) { this.collection .indexOfFirst { it.item.storyId == id && it.item.type == CardComponentResponse.Type.COPY_SPREAD } .let { recyclerView.post { notifyItemChanged(it, CopySpreadEvent.CardViewed()) } if (it > 0) { collection[it] } else { null } } ?.item ?.hasBeenViewed = true } 

Chaining fun RendererAdapter>.markAsViewed(recyclerView: RecyclerView, id: CardId) { val firstIndex = this.collection.indexOfFirst { it.item.storyId == id && it.item.type == CardComponentResponse.Type.COPY_SPREAD } recyclerView.post { notifyItemChanged(firstIndex, CopySpreadEvent.CardViewed()) } if (firstIndex > 0) collection[firstIndex].item.hasBeenViewed = true } 

Chaining 

When 

Not an If when (enabled) { true -> view.enableSend() false -> view.disableSend() } if (enabled) { view.enableSend() } else { view.disableSend() } 

Compile time check https://goo.gl/NSXJEA fun access(employee: Employee, contract: Contract): SafariBookAccess { return when (employee) { SENIOR_ENGINEER -> when (contract) { PROBATION -> NotGranted(AssertionError("Not allowed for probation.")) PERMANENT -> Granted(DateTime()) CONTRACTOR -> Granted(DateTime()) } REGULAR_ENGINEER -> when (contract) { PROBATION -> NotGranted(AssertionError("Not allowed for probation.")) PERMANENT -> Granted(DateTime()) CONTRACTOR -> Blocked("Access blocked for $contract.") } JUNIOR_ENGINEER -> when (contract) { PROBATION -> NotGranted(AssertionError("Not allowed for probation.")) PERMANENT -> Blocked("Access blocked for $contract.") CONTRACTOR -> Blocked("Access blocked for $contract.") } else -> throw AssertionError() } } 

Compile time check https://goo.gl/NSXJEA fun access(contract: Contract, employee: Employee) = when (Pair(contract, employee)) { Pair(PROBATION, SENIOR_ENGINEER), Pair(PROBATION, REGULAR_ENGINEER), Pair(PROBATION, JUNIOR_ENGINEER) -> NotGranted(AssertionError("Not allowed for probation.”)) Pair(PERMANENT, SENIOR_ENGINEER), Pair(PERMANENT, REGULAR_ENGINEER), Pair(PERMANENT, JUNIOR_ENGINEER), Pair(CONTRACTOR, SENIOR_ENGINEER) -> Granted(DateTime(1)) Pair(CONTRACTOR, REGULAR_ENGINEER), Pair(CONTRACTOR, JUNIOR_ENGINEER) -> Blocked("Access for junior contractors is blocked.") else -> throw AssertionError("Unsupported case of $employee and $contract") } 

Compile time check https://goo.gl/NSXJEA fun access(contract: Contract, employee: Employee) = when (Pair(contract, employee)) { Pair(PROBATION, SENIOR_ENGINEER), Pair(PROBATION, REGULAR_ENGINEER), Pair(PROBATION, JUNIOR_ENGINEER) -> NotGranted(AssertionError("Not allowed for probation.”)) Pair(PERMANENT, SENIOR_ENGINEER), Pair(PERMANENT, JUNIOR_ENGINEER), Pair(CONTRACTOR, SENIOR_ENGINEER) -> Granted(DateTime(1)) Pair(CONTRACTOR, REGULAR_ENGINEER), Pair(CONTRACTOR, JUNIOR_ENGINEER) -> Blocked("Access for junior contractors is blocked.") else -> throw AssertionError("Unsupported case of $employee and $contract") } 

Compile time check https://goo.gl/NSXJEA fun access(contract: Contract, employee: Employee) = when (Pair(contract, employee)) { Pair(PROBATION, SENIOR_ENGINEER), Pair(PROBATION, REGULAR_ENGINEER), Pair(PERMANENT, JUNIOR_ENGINEER), Pair(PROBATION, JUNIOR_ENGINEER) -> NotGranted(AssertionError("Not allowed for probation.”)) Pair(PERMANENT, SENIOR_ENGINEER), Pair(PERMANENT, JUNIOR_ENGINEER), Pair(CONTRACTOR, SENIOR_ENGINEER) -> Granted(DateTime(1)) Pair(CONTRACTOR, REGULAR_ENGINEER), Pair(CONTRACTOR, JUNIOR_ENGINEER) -> Blocked("Access for junior contractors is blocked.") else -> throw AssertionError("Unsupported case of $employee and $contract") } 

Extensions Let’s not fall into Extensions Hell while complaining about Utils Hell :) 

Unique Naming 

Unique Naming Conflicting overloads: public fun String.check(): Boolean defined in com.xing.sample.talk in file VerificationExtension.kt, public fun String.check(): Boolean defined in com.xing.sample.talk in file NamingExtension.kt 

Top level const const val FIELD = "field" 

Make them • private or internal Use companion object • Keep only global ones as top level • Constants 

No Java in Kotlin Especially with null checks 

fun transformToRepoItem(repo: Repo): RepoItem { val item = RepoItem() if (repo.name != null) item.name = repo.name!! if (repo.description != null) item.description = repo.description!! if (repo.owner != null && repo.owner!!.login != null) item.login = repo.owner!!.login!! if (repo.fork != null) item.fork = repo.fork!! if (repo.htmlUrl != null) item.repoURL = repo.htmlUrl!! if (repo.owner != null && repo.owner!!.htmlUrl != null) item.ownerURL = repo.owner!!.htmlUrl!! return item } 

fun transformToRepoItem(repo: Repo): RepoItem { return RepoItem().apply { name = repo.name ?: "" description = repo.description ?: "" login = repo.owner?.login ?: "" fork = repo.fork ?: false repoURL = repo.htmlUrl ?: "" ownerURL = repo.owner?.htmlUrl ?: "" } } 

Find the Kotlin way! But still keep it readable, for next person. Thanks