Readable codebase: extract conditions / predicates

A quick way to make a codebase more readable is to extract long or complex conditions / predicates to their own functions. The function’s name will explain to the reader what is being checked allowing her to move forward instead of trying to figure out the context or the calculations that are taking place.

The task

enum class Status {
NotStarted,
InProgress,
Resolved,
Cancelled
}
class Task(
val id: Int,
val description: String,
val status: Status,
val createdAt: LocalDateTime
)

Completed tasks of the last two days

Lets assume that we have a piece of code that prints all tasks that were created in the last two days and are, as the business experts calls them, completed. Which means that are either Resolved or Cancelled.

val yesterdayAtStartOfDay = LocalDate.now().minusDays(1).atStartOfDay()
val todayAtEndOfDay = LocalDate.now().atTime(23, 59)
allTasks
.filter { task >
(task.createdAt > yesterdayAtStartOfDay && task.createdAt < todayAtEndOfDay)
&& (task.status == Resolved || task.status == Cancelled)
}
.forEach { task > println(task.description) }

We have two distinct concepts that can be extracted. One that will explain the date calculations and one that will force the business knowledge behind those two statuses.

createdInTheLastTwoDays()

The calculations can be extracted to a private function but since we are using Kotlin we will extract them to an extension function to make the code even more readable:

allTasks
.filter { task >
task.createdInTheLastTwoDays()
&& (task.status == Resolved || task.status == Cancelled)
}
.forEach { task > println(task.description) }
// extracted code:
private fun Task.createdInTheLastTwoDays(): Boolean {
val yesterdayAtStartOfDay = LocalDate.now().minusDays(1).atStartOfDay()
val todayAtEndOfDay = LocalDate.now().atTime(23, 59)
return this.createdAt > yesterdayAtStartOfDay && this.createdAt < todayAtEndOfDay
}

isCompleted()

Another benefit of having “the mentality of extraction” (yep, I just made it up) is that eventually we will come across a case like the completed which is a business term that has not been transferred to the codebase!

So we don’t just make the code more readable but we improve our domain representation and force a rule that until now was known through documentations or, even worse, through conversations only.

allTasks
.filter { task > task.createdInTheLastTwoDays() && task.isCompleted() }
.forEach { task > println(task.description) }
//extracted code:
class Task(
val id: Int,
val description: String,
val status: Status,
val createdAt: LocalDateTime
) {
fun isCompleted(): Boolean {
return status == Resolved
|| status == Cancelled
}
}

The reader will not have to make any calculations while reading this code or trying to understand why only those statuses are being checked.

The code speaks for itself!

Write comments only when they answer the “Why?”

This is the one and only question that I ask myself when I see comments or when I feel that I need to write some:

Do these comments provide the reason why this code was written?

If not then 99.9% we don’t need them.

“Remove right margin”

I recently came across a block of code that looked like this:

private fun foo(ui: Ui, system: System) {
val isGridLayout: Boolean = ui.itemsAreInGrid()
// Remove right margin when the theme is light and the layout is a list
if (!isGridLayout && !system.isDarkThemeEnabled()) {
val uiContainer = ui.container
uiContainer.rightMargin = 0
}
}

In this example the comment simply describes what the code does. I am pretty sure that everyone can figure that out but only the code’s author knows why we need to remove the margin under those circumstances. I am also sure that in six months (in my case in six days) even the author won’t remember the reason behind this code.

Extract if’s logic

To be fair I do understand the author’s urge to write this comment. Those unavoidable negations, because of the system’s/ui’s APIs, look like they need to be clarified.

Fortunately there is a better way to do that: we extract if’s logic to its own method and name it accordingly:

private fun foo(ui: Ui, system: System) {
// Remove right margin when the theme is light and the layout is a list
if (inLightModeWithItemsInList(ui, system)) {
val uiContainer = ui.container
uiContainer.rightMargin = 0
}
}
private fun inLightModeWithItemsInList(ui: Ui, system: System): Boolean {
return !ui.itemsAreInGrid() && !system.isDarkThemeEnabled()
}

Extract if’s body

We can go a step further and extract if’s body to its own method too, having the code replacing the comment completely:

private fun foo(ui: Ui, system: System) {
if (inLightModeWithItemsInList(ui, system)) {
removeRightMargin(ui)
}
}
private fun removeRightMargin(ui: Ui) {
val uiContainer = ui.container
uiContainer.rightMargin = 0
}
private fun inLightModeWithItemsInList(ui: Ui, system: System): Boolean {
return !ui.itemsAreInGrid() && !system.isDarkThemeEnabled()
}

Answer the “Why?”

So now what is missing is the reason behind the margin’s removal. A comment that answers the why and completes the reader’s understanding about the code she/he reads.

private fun foo(ui: Ui, system: System) {
if (inLightModeWithItemsInList(ui, system)) {
// blah blah …
removeRightMargin(ui)
}
}

“I can see what is happening and I know why too!”

A function’s intent should be revealed by its name

And a good way to know if it doesn’t is to read it where it gets used. If, after reading it, you have questions then something is wrong with the name.

A good example is this code that I run into:

if (handleClickItem(customer)) {
    return;
}

When I read this piece of code the very first thing that popped into my head was

How does the click gets handled?!

To figure that out I had to step into the function’s body and start reading it in order to understand what it does and when. It broke my flow and made me change context.

Turned out that this particular function, depending on the customer’s type, opens the contact’s screen and returns true or simply returns false. Having read something like:

if (openContactsScreenWhenCustomerIsCompany(customer)) {
    return;
}

would have been enough for me to simply keep reading and never look back.

Use sealed classes for better domain representation

Lets start with the business logic.

We have a task. A task can be unassigned OR it can be assigned either to a user OR a group.

First implementation: the ugly way

One way to implement this is by putting all the logic in the task:

class UglyTask(
  val name: String,
  val assignedUser: User? = null,
  val assignedGroup: Group? = null
) {
  init {
  if (assignedUser != null && assignedGroup != null) {
  throw IllegalArgumentException("a task can be assigned to either a user OR a group.")
  }
  }
}
view raw uglytask.kt hosted with ❤ by GitHub

By default the task is unassigned and if we want an assigned task we provide a user or a group. The or factor is being enforced by a check in the constructor.

This implementation not only relies on nulls to represent the business logic but it also hides the logic from the developer who has to read the code to understand how to create an assigned task.

The null checking comes also up when we want to figure out if and where a task is assigned which is tedious and can easily lead to bugs. As an example lets consider a simple function that prints the task’s assignment state:

fun UglyTask.printAssignment() {
when {
assignedGroup == null && assignedUser == null > println("\"$name\" is assigned to no one")
assignedGroup != null > println("\"$name\" is assigned to to group: ${assignedGroup.name}")
assignedUser != null > println("\"$name\" is assigned to to user: ${assignedUser.name}")
}
}

Finally two points that we should not neglect are readability and scalability. When using UglyTask if we want our code to be readable, in all cases, we have to pass the arguments by their names (thank you Kotlin 🙂 ):

val le0nidas = User("le0nidas")
val kotlinEnthusiasts = Group("kotlin enthusiasts")
UglyTask("buy milk").printAssignment()
UglyTask("write post", assignedUser = le0nidas).printAssignment()
// here we pass the parameter by name to avoid the usage of null
// which makes it less readable
UglyTask("write kotlin", assignedGroup = kotlinEnthusiasts).printAssignment()
UglyTask("write kotlin", null, kotlinEnthusiasts).printAssignment()
view raw uglytask_usage.kt hosted with ❤ by GitHub

As far as scalability, consider how many changes we need to do to add a new assigned entity. One to the constructor, one to the init function to enforce our business logic and one in every function that we use the task’s state (see: printAssignment()) which adds even more null checks.

Second implementation: The less ugly way

Another way is by having multiple constructors, each for every valid assignment:

class LessUglyTask private constructor(
val name: String,
val assignedUser: User?,
val assignedGroup: Group?
) {
constructor(name: String) : this(name, null, null) // assigned to no one
constructor(name: String, assignedUser: User) : this(name, assignedUser, null) // assigned to a user
constructor(name: String, assignedGroup: Group) : this(name, null, assignedGroup) // assigned to a group
}
view raw lessunglytask.kt hosted with ❤ by GitHub

This implementation also puts all the logic in the task but it removes those null checks and makes it easier for the developer to understand it:

With that said, all other drawbacks in readability and scalability remain the same:

fun LessUglyTask.printAssignment() {
when {
assignedGroup == null && assignedUser == null > println("\"$name\" is assigned to no one")
assignedGroup != null > println("\"$name\" is assigned to to group: ${assignedGroup.name}")
assignedUser != null > println("\"$name\" is assigned to to user: ${assignedUser.name}")
}
}
val le0nidas = User("le0nidas")
val kotlinEnthusiasts = Group("kotlin enthusiasts")
LessUglyTask("buy milk").printAssignment()
LessUglyTask("write post", assignedUser = le0nidas).printAssignment()
LessUglyTask("write kotlin", assignedGroup = kotlinEnthusiasts).printAssignment()

Final implementation: the sealed classes way 🙂

The best way to implement the business logic is by using Kotlin’s sealed classes. This way we can represent our business logic straight into our code and also keep our code clean, readable and scalable:

sealed class AssignedTo
object AssignedToNoOne : AssignedTo()
data class AssignedToUser(val user: User) : AssignedTo()
data class AssignedToGroup(val group: Group) : AssignedTo()
class Task(val name: String, val assignedTo: AssignedTo)
view raw task.kt hosted with ❤ by GitHub

Now, printAssignment() leverages all of Kotlin’s powers, including smart cast, making it easier to the eye:

fun Task.printAssignment() {
when (assignedTo) {
is AssignedToNoOne > println("\"$name\" is assigned to no one")
is AssignedToGroup > println("\"$name\" is assigned to ${assignedTo.group.name}")
is AssignedToUser > println("\"$name\" is assigned to ${assignedTo.user.name}")
}
}

and the rest of the code does not need any extra help like named arguments:

val le0nidas = User("le0nidas")
val kotlinEnthusiasts = Group("kotlin enthusiasts")
Task("buy milk", AssignedToNoOne).printAssignment()
Task("write post", AssignedToUser(le0nidas)).printAssignment()
Task("write kotlin", AssignedToGroup(kotlinEnthusiasts)).printAssignment()
view raw task_usage.kt hosted with ❤ by GitHub

As for scalability, when we want to add a new way of assignment we just extend AssignedTo and we are good to go.