TIL: @NullAndEmptySource in JUnit5

This is a clear case of RTFM!

I wanted to make sure that a function will return null when given a null or empty string and what I ended up doing was something like this:

	internal class CreateNameTest {

	@ParameterizedTest
	@ValueSource(strings = ["null", "", " "])
	fun `there is no creation when the provided value is null or empty or blank`(providedValue: String?) {
	val value: String? = if (providedValue == "null")
	null else
	providedValue
	assertThat(createName(value), absent())
	}
	}

view raw initial_parameterized_test.kt hosted with ❤ by GitHub

@ValueSource does not accept null values so I passed it indirectly!

I didn’t like it so after, finally, reading the JUnit5 documentation I learned that the library had me covered from the beginning by providing three annotations exactly for this use case:

@NullSource, @EmptySource and @NullOrEmptySource are meant to be used whenever we need to check the behavior of our code when given null or empty inputs.

So the test changes to:

	internal class CreateNameTest {

	@ParameterizedTest
	@NullAndEmptySource
	@ValueSource(strings = [" "])
	fun `there is no creation when the provided value is null or empty or blank`(providedValue: String?) {
	assertThat(createName(providedValue), absent())
	}
	}

view raw parameterized_test_with_proper_annotations.kt hosted with ❤ by GitHub

PS: for the curious, the absent() is part of the hamkrest library by Nat Pryce (yes, that Nat Pryce)

A use case of using chain of responsibility pattern to scale strategy

Lets say we have a simple application that executes certain actions dictated by an external service.

More particular the external service sends a response that looks like this:

	class Response(
	val action: String,
	val value: String
	)

view raw response_first_iteration.kt hosted with ❤ by GitHub

the supported actions are:

	interface Action {
	val name: String
	fun execute(response: Response)
	}

	class Print : Action {

	override val name: String = "print"

	override fun execute(response: Response) {
	println("printing ${response.value}…")
	}
	}

	class Log : Action {

	override val name: String = "log"

	override fun execute(response: Response) {
	println("logging ${response.value}…")
	}
	}

	class WriteToFile : Action {

	override val name: String = "write_to_file"

	override fun execute(response: Response) {
	println("writing to file ${response.value}…")
	}
	}

view raw actions_first_iteration.kt hosted with ❤ by GitHub

and there is an executor to help as translate the response to an actual action execution:

	class ActionExecutor {

	private val allActions = mutableMapOf<String, Action>()

	fun addAction(action: Action) {
	allActions[action.name] = action
	}

	fun executeByResponse(response: Response) {
	val action = allActions[response.action]
	action?.execute(response)
	}
	}

view raw action_executor_first_iteration.kt hosted with ❤ by GitHub

This is an implementation of the Strategy Pattern where every strategy is a certain action allowing us to add as many actions as we want as long as there is a one-to-one relation with what the response sends:

	fun main() {
	val executor = createActionExecutor()

	executor.executeByResponse(Response("print", "leonidas"))
	executor.executeByResponse(Response("log", "kotlin"))
	executor.executeByResponse(Response("write_to_file", "leonidas loves kotlin"))
	}

	private fun createActionExecutor(): ActionExecutor {
	return ActionExecutor().apply {
	addAction(Print())
	addAction(Log())
	addAction(WriteToFile())
	}
	}

view raw main_first_iteration.kt hosted with ❤ by GitHub

The problem

One day the external service decides to add a print error action but it does not do it by having a new action value. Instead it decides to have an is error flag which must be taken under consideration when the action value is print!

So the response is now this:

	class Response(
	val action: String,
	val value: String,
	val isError: Boolean = false
	)

view raw response_second_iteration.kt hosted with ❤ by GitHub

leading us into having an if in the print action’s code

	class Print : Action {

	override val name: String = "print"

	override fun execute(response: Response) {
	if (response.isError) {
	System.err.println("printing (in error stream) ${response.value}…")
	return
	}
	println("printing ${response.value}…")
	}
	}

view raw print_second_iteration.kt hosted with ❤ by GitHub

which defeats the purpose of the strategy pattern and violates the SRP. Each strategy should do one thing and one thing only. By having an if in the strategy we allow it to implement two actions thus having two reasons to change.

Adding a chain

What we need is a way to allow the representation of a single action value from multiple strategies where each one implements a unique action depending on the values of the response.

This is where the Chain of Responsibility Pattern comes in helping as into having multiple actions tied together, passing the response to each other until one of them can execute it.

There are two ways to implement the pattern depending on the size and state of your project.

Using inheritance

If you have a small project with just a few actions and you don’t mind touching many files you can use inheritance and have each action containing the next in the chain:

	abstract class BaseAction(
	private val nextAction: Action?
	) : Action {

	override fun execute(response: Response) {
	nextAction?.execute(response)
	}
	}

	class Print : Action {

	override val name: String = "print"

	override fun execute(response: Response) {
	println("printing ${response.value}…")
	}
	}

	class PrintInErrorStream(
	nextAction: Action?
	) : BaseAction(nextAction) {

	override val name: String = "print"

	override fun execute(response: Response) {
	if (response.isError) {
	System.err.println("printing (in error stream) ${response.value}…")
	return
	}
	super.execute(response)
	}
	}

view raw cof_inheritance.kt hosted with ❤ by GitHub

which can be used as such:

	fun main() {
	val executor = createActionExecutor()

	executor.executeByResponse(Response("print", "leonidas"))
	executor.executeByResponse(Response("log", "kotlin"))
	executor.executeByResponse(Response("write_to_file", "leonidas loves kotlin"))
	executor.executeByResponse(Response("print", "$&#$@#", isError = true))
	}

	private fun createActionExecutor(): ActionExecutor {
	return ActionExecutor().apply {
	addAction(PrintInErrorStream(Print())) // first try to print in error and the in simple
	addAction(Log())
	addAction(WriteToFile())
	}
	}

view raw main_cor_inheritance.kt hosted with ❤ by GitHub

Using composition

If we do not wish to change any of the existing actions then composition is the only way.

What we need is a way to wrap an existing action in an object that will (a) hold the next action in the chain and (b) decide which action will be executed, the wrapped or the next one:

	class ChainAction(
	private val action: Action,
	private val canExecute: (Response) -> Boolean
	) : Action {

	private var nextAction: Action? = null
	override val name: String = action.name

	override fun execute(response: Response) {
	if (canExecute(response)) {
	action.execute(response)
	return
	}

	nextAction?.execute(response)
	}

	fun or(action: ChainAction): ChainAction {
	nextAction = action
	return this
	}
	}

	fun Action.asChain(canExecute: (Response) -> Boolean): ChainAction =
	ChainAction(this, canExecute)

view raw chain_action.kt hosted with ❤ by GitHub

Using ChainAction results in this:

	fun main() {
	val executor = createActionExecutor()

	executor.executeByResponse(Response("print", "leonidas"))
	executor.executeByResponse(Response("log", "kotlin"))
	executor.executeByResponse(Response("write_to_file", "leonidas loves kotlin"))
	executor.executeByResponse(Response("print", "$&#$@#", isError = true))
	}

	private fun createActionExecutor(): ActionExecutor {
	val printInErrorStream = PrintInErrorStream().asChain { response -> response.isError }
	val justPrint = Print().asChain { true }

	return ActionExecutor().apply {
	addAction(printInErrorStream.or(justPrint))
	addAction(Log())
	addAction(WriteToFile())
	}
	}

view raw main_cor_composition.kt hosted with ❤ by GitHub

This way we can scale the strategy pattern both vertically, one strategy per action value, and horizontally, one strategy per action value sub cases.

I wrote a GitHub Action using Kotlin

I decided to take a look at GitHub Actions so for the past week I’ve been watching and reading everything about it. I even wrote a post, an Introduction to GitHub Actions. What I found really interesting is the fact that you can write an action using the language you feel more comfortable with. And so I did!

I wrote a small action that:

collects all the changed files from a PR,
keeps those that have the .kt suffix,
runs ktlint on them and
makes a comment in the PR for every error that ktlint reports

and all that using Kotlin and some bash!

You can find and use it here: ktlint-pr-comments

GitHub Action using Docker

There are three ways to create an action but only the one using Docker allows us to use the language we want.

In all three ways the main two ingredients are:

the action’s code
the action’s metadata, a file called action.yml which is placed in the root folder of your project and defines how the action will run and what inputs/outputs it has.

In our case there is also a third ingredient, a Dockerfile or a docker image which is used by the action’s runner to create a container and execute the action’s code inside it. All you have to do is to make sure that the action’s executable parts are being copied in the container and that are called upon its start.

The runner makes sure that the working space is being mounted to the container (in the state that it was just before the action is started) along with all the environment variables and the inputs the action needs. You can read more in the documentation.

Ktlint PR comments

Action’s code

The action has three distinct parts.

The first part is responsible for using GitHub’s REST API to collect all of the PR’s changes and then keep those that are in Kotlin files and were added or modified. For that I used kscript and I was able to leverage all the libraries that I was accustomed to, like Retrofit and Moshi. When I was happy with the resulted script I used its --package option to create a standalone binary and copy it in the action’s Docker image.

The second part is a combination of bash commands that execute the ktlint binary by passing to it the results of the first part. Ktlint is being called with the --reporter=json parameter in order to create a JSON report.

The third and final part is again a kscript script that uses the report created before and GitHub’s REST API to make a PR line comment for every ktlint error that is part of the PR’s diff. Again a standalone binary was created and put in the image.

Note:

I like kscript since I can write things fast, easy and with all the libraries that I know but I also like writing test first and that proved to be quite difficult. So what I ended up doing was to act as if I was in a Kotlin project. I created my tests (using all my favorites like junit5, hamkrest and MockWebServer) and from that I created .kt files with the proper functionality. And for having a script I created a .kts file where I defined the external dependencies and included the .kt files:

	import kotlin.system.exitProcess

	//DEPS com.squareup.moshi:moshi:1.9.3
	//DEPS com.squareup.moshi:moshi-kotlin:1.9.3
	//DEPS com.squareup.retrofit2:retrofit:2.9.0
	//DEPS com.squareup.retrofit2:converter-moshi:2.9.0

	//INCLUDE logging.kt
	//INCLUDE common.kt
	//INCLUDE collectPrChanges.kt
	//INCLUDE createGithubEvent.kt

	val result = collectPrChanges(args)

	if (result != 0) {
	exitProcess(result)
	}

	println("Changes collected")

view raw exec-kscript.kts hosted with ❤ by GitHub

Action’s metadata

From the start what I wanted for this action was to be as autonomous as possible leaving very little responsibilities to the consumer and allowing her to just plug it in and watch it play.

For that the only input the action needs is a token, for allowing the kscript scripts to communicate with the API, which will most likely be the default secrets.GITHUB_TOKEN making the action’s usage as simple as adding the following lines in your workflow:

- uses: le0nidas/ktlint-pr-comments@v1
  with:
    repotoken: ${{ secrets.GITHUB_TOKEN }}

Docker image

A lot of things must happen in order to have the action ready to run. Sdkman, Kotlin and kscript must be installed, the code needs to be retrieved from the repository and both kscript scripts have to be packaged. On top of that ktlint must be downloaded and placed in the proper path.

For all that, and to shave a few seconds from the action, I decided to have an image that has everything ready. So I created a workflow that gets triggered every time there is a push in the main branch, builds and packs everything in an image and pushes the result to Docker Hub.

So now the action simply uses that image to run a container without any other ceremonies.

Note:

Before using Docker Hub I tried to use GitHub Packages but it turns out that public is not that public^* since it requires an authentication to retrieve a package.

Summary

That’s it! An action for having ktlint’s report as comments in your PR. A result of trial and error since I wrote it while learning about actions but I hope that someone might find it useful. If you do let me know!

An example of how to use it can be found in the action’s repository where I dogfood it to the project.

* [3 Sep 2020]: looks like things will change

Introduction to GitHub Actions

Github actions is a way to execute one or more tasks every time a certain event occurs. For example, setting reviewers (the task) every time a PR is being opened (the event).

How do I use them?

By creating workflows. A workflow is a .yml file that (a) defines the events that we wish to listen for and (b) describes the jobs that are going to be executed. A job is the task that was mentioned earlier and consists from one or more of steps. Now a step is either a command or an action but more on that in a bit.

Lets see a simple workflow to clarify those definitions:

	name: Check PR

	on:
	pull_request:
	types: [opened, synchronize]

	jobs:
	check-pr-quality:
	runs-on: ubuntu-latest

	steps:
	– run: echo "Get code from repository"
	– run: echo "Setup JDK 1.8"
	– run: echo "Make gradlew executable"
	– run: echo "Compile project"
	– run: echo "Run tests"
	– run: echo "Run quality tools"

	set-reviewer:
	runs-on: ubuntu-latest

	steps:
	– run: echo "Check availability"
	– run: echo "Set first available"

view raw demo-workflow.yml hosted with ❤ by GitHub

This is a workflow named Check PR that runs every time there is a new or updated PR and executes two jobs (here in parallel but can be configured to execute them sequentially):

The first job, named check-pr-quality, has 6 steps and in each one runs the echo command to print a message.
The second job, named set-reviewer, has 2 steps and, as in the first one, it runs the echo command in each step to print a message.

Admittedly, this is not a valuable workflow but it is a valid one! Just copy and paste it in a .yml file inside your repository under the .github/workflows folder (this is where all workflows should be added) and then create a new PR. After pushing it to GitHub go to the Actions tab you will see something similar to this:

What’s happening under the hood is that for each job:

a new and clean instance of Ubuntu (configured by runs-on key) is being boot up in a virtual machine
some necessary environment variables are being set up, and
we get “landed” in the /home/runner/work/our-repo-name/our-repo-name (no need to remember it, you can use the GITHUB_WORKSPACE environment variable) path ready to start executing the defined steps

Steps

As mentioned earlier a step can either be a command or an action.

A command is everything that can be executed inside the OS. Let’s not forget that we are inside a virtual machine with various tools installed. We can run from simple bash commands like the one we saw (echo) to multiple ones that help as install and run extra software. For example:

- run: |
    curl -s "https://get.sdkman.io" | bash
    source "$HOME/.sdkman/bin/sdkman-init.sh" && sdk install kotlin

this snippet installs sdkman and then uses it to install kotlin. So now our Ubuntu instance has Kotlin!

An action is what we’ll use when a couple of commands won’t be enough. Think of an action like a full fledged program that can have inputs, produce outputs and can be programmed to do pretty much whatever we want.

Lets change the first job in the workflow above by replacing all echo commands with actions and commands that will actually do what was previously just described:

	name: Check PR

	on:
	pull_request:
	types: [opened, synchronize]

	jobs:
	check-pr-quality:
	runs-on: ubuntu-latest

	steps:
	– name: Get code from repository
	uses: actions/checkout@v2

	– name: Setup JDK 1.8
	uses: actions/setup-java@v1
	with:
	java-version: 1.8

	– name: Make gradlew executable
	run: chmod +x gradlew

	– name: Compile project
	run: ./gradlew assemble

	– name: Run tests
	run: ./gradlew test

	– name: Run quality tools
	uses: le0nidas/ktlint-pr-comments@v1
	with:
	repotoken: ${{ secrets.GITHUB_TOKEN }}

view raw working-workflow.yml hosted with ❤ by GitHub

As before we have a workflow which executes a job that consists from 6 steps.

Only this time we use an action to download our repository to GITHUB_WORKSPACE, an action to setup our java environment, a couple of commands to compile and test our project now that we have it in our workspace (gradlew is part of our project) and an action that will run ktlint and make comments on our PR if there are any errors.

Summary

Think of it like being in a terminal, running and combining multiple commands to produce a result. A job is that terminal and actions are those commands. Mix and match them to automate whatever takes time in your processes.

For more on GitHub actions you can read its documentation, do some of the official training or start reading the code of all those available actions in the marketplace!

Know your tools: scratch files in IntelliJ IDEA

I’ve used scratch files in IntelliJ IDEA and Android Studio but I think that can be found in all of Jetbrain’s products.

What are they?

Scratch files are files that don’t get tracked by the version control system, can be created at any given time and, most importantly, get bind to the IDE and not the project that is currently open.

How do I create them?

The simplest way is to hit ctrl+alt+shift+insert. If you can’t remember it press shift twice and start writing scratch, you will be presented with the action of creating a new one.

The next step is to choose what kind of file you want to create and this is where it gets interesting since you can choose from a plethora of file types. From plain text, to markdown, Kotlin, JSON, XML, ruby and many many more!

How do I use them?

By choosing the file’s type you choose how the IDE will behave when you are working on it, so if you create a scratch.json and paste some json in it you can format it accordingly. Or if you create a scratch.md you can start writing in markdown and have a preview of your work.

But the most powerful aspect of those files is when you create code related ones. If, for example, you create a scratch.kts file and start writing some Kotlin in it, you will see your code being run on the fly presenting to you its result:

TDDish

You can even work test first if you need to figure out a quick algorithm and have your test run in every change you make!

I usually start with an assertThat function and a failing test and go from there:

Its a simple one but you get the point:

TIL: vararg in Kotlin is never nullable

Today I came across a piece of code that looked like this:

fun printAll(vararg names: String?) {
  names.forEach { name -> println(name) }
}

and noticed that the IDE did not complain about using names without checking if it is null names?.forEach { ... }!

After decompiling Kotlin’s bytecode I saw that no matter what type I use (String? or String) the java code was the same:

public static final void printAll(@NotNull String... languages) {
  //...
}

Does the Kotlin compiler ignore the nullable type completely? Turns out that yes!

And the reason is quite clear and straightforward but it hadn’t registered in my mind until now:

Note that vararg parameters are, as a rule, never nullable, because in Java there is no good way to distinguish between passing null as the entire vararg array versus passing null as a single element of a non-null vararg array.
kotlin’s forums

What if we pass a null value?

As a matter of fact if you pass a null value:

printAll(null)

the compiler makes sure that the java code will be called with a null value casted to the appropriate type:

printAll((String)null);

which ends up in an array of strings that has one element!

kscript from docker

This is one of those cases that you go down the rabbit hole and end up doing ten different things before the one that you initially wanted to!

My original intention was to try out kscript. What I ended up doing is learning about dockerfiles, building images and trying to run .kts files without having kscript, or kotlin, installed locally in my machine!

The result

A github repository that you can clone, run the install script and have a kscript executable that works as described here.

Why?

I wanted to play with kscript but did not want to install it on my machine. I don’t like bloating my OS by having libraries and software that might not be used often.

How?

Having everything installed in a docker image and use containers to play with kscript was a one way street for what I wanted to achieve.

Dockerfile

It was my first dockerfile but there are tons of tutorials that helped me out. I ended up using alpine, installing java through its repositories, sdkman using its installation guide and kotlin, kscript through sdkman!

Finally, kscript was added as the image’s entry point which means that every time a container is being run kscript will be the first command that gets executed.

And it worked for the most parts. By executing

docker run –rm -i kscript <params>
rm to remove the container after its done, -i to have the script’s output passed to the terminal

I was able to use kscript as described in its repository, except for the case of having a .kts file!

kscript-router.sh

For the case of a .kts file what turned out to solve my problem was to pass the file’s entire content. And this is part of what kscript-router.sh does. It checks if the first parameter is a .kts file and if so it passes its contents to the container. In every other case it passes all given parameters directly to the container.

install.sh

To tie everything together I added a script that creates the image, adds the router script to the user’s path and creates an alias, named kscript, for executing the router.

kscript-from-docker

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
	)

view raw extact-predicate-initial-code.kt hosted with ❤ by GitHub

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) }

view raw extract-predicate-initial-code-filter.kt hosted with ❤ by GitHub

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
	}

view raw extract-predicate-created-at-extraction.kt hosted with ❤ by GitHub

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
	}
	}

view raw extract-predicate-completed-extraction.kt hosted with ❤ by GitHub

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
	}
	}

view raw commet-only-why-initial-code.kt hosted with ❤ by GitHub

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()
	}

view raw commet-only-why-if-logic.kt hosted with ❤ by GitHub

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()
	}

view raw commet-only-why-if-body.kt hosted with ❤ by GitHub

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)
	}
	}

view raw commet-only-why-the-comment.kt hosted with ❤ by GitHub

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

Code review: don’t just request a change

When reviewing a PR I try to elaborate on my proposals for two reasons:

1. Out of respect to my colleague

Every piece of code is part of an effort that took time and thought. Requesting a code change by simply asking it (i don't like it, change it) or by dictating it (do it this way) demotes all the work that has been done.

I figured that, since my colleague spent a few hours to come up with a solution I owe her/him more than a few seconds!

2. I solidify my knowledge or even better, learn something new

By trying to justify the reason behind a request I end up understanding better why I prefer a solution or a format over another.

When I provide an example or when I do a mini investigation to help me clarify why something must change I often find that (a) many things that I thought I knew were not as I had them in mind and (b) I now understand a concept much better because I had to write about it. You don’t master something if you can’t explain it!

PS: never forget that it might say “request a change” but in reality you start a conversation between two professionals that will eventually benefit of the project