Humans are complex

we are all different

Communication is difficult

appreciation and criticism even more

We tend to be too creative

Too much effort in Code Reviews

We're not very good at revewing code

Code Reviews should not block or slow down development

Code Reviews should as short and simple as possible

Code Reviews should be fast

Code Reviews should build trust

Code Reviews provide learning opportunities

Everyone is accountable

Code Reviews are not a subject of preference

Aspects of software design are almost never a pure style issue or just a personal preference

Good communication is key

“There is no such thing as “perfect” code - there is only better code. Instead of seeking perfection, what a code review (Reviewer) should seek is continuous improvement.”

All code is garbage ™

Functionality ⚙️

The functionality is good for the users of the code?

Design 🎨

The code is well-designed?

Complexity 🤯

The code is more complex than it needs to be?

Tests & Coverage 🧪

Code has appropriate tests and tests are well designed?

Comments 💬

Are all of the comments actually necessary and explaning the why?

Style 😎

The code conforms to project style guidelines?

Consistency 🤓

Code is consistent with the recommendations or the surrounding code?

Documentation 🖹

Code is appropriately documented?

Review Principles

Design • Functionality • Comments • Style • Consistency • Documentation • Complexity • Tests & Coverage

Make good use of git

Use a review tool

It will prefil automatically the Code Review details...

Invite the right Audience

• Codebase owners
• Known subject-matter experts
• Interested parties

Make sure audience is expecting the changes

Avoid surprises...

Be nice kind

• Provide evidences (screenshots, logs, etc)
• Leave any comments of your own

What is this about?

Look at the Code Review description, the commit and what the code does in general. Does this change even make sense?

Reject the change

If this change shouldn't have happened in the first place, please respond immediately with an explanation of why the change should not be happening.

Comment Etiquette

When you reject a change like this, it's important suggest to the developer what they should have done instead and reject the code.

• Don't assume the obvious
• Ask questions
• Be nice kind
• Be courteous and respectful
• Consider differents point of view

Request Changes ⚠️

“There was a recent announcement that the library mapping4js won't be supported anymore from version 2.5. (circumstance)

I'm afraid this is not the best way to go. I'm aware of the current limitation of the existing mapping implementation (...) (acknowledge)

but I was just having a look on this other library (...), seems to be a better approach in the long term. What to you think? (guidance/openness)

Btw, I really liked the idea of replacing the custom library! (positivity)”

Request Changes ⚠️

“I found this idea really interesting but I'm afraid that doing such a risky change at this specific time will impact delivery of other features currently being implemented. (circumstance) Your point in adding this library is totally valid (acknowledge), but I was having a look and I found this alternative Here's an how we (teamwork) could do possibly achieve a good result:”

						
							// form-validator.js
							const complex = (typeA, typeB) => { typeA, typeB }

							// validations.js
							address: use(complex)
						
					

“What do you think if we proceed with this workaround for now and add the library later? (guidance)”

Request Changes ⚠️

“At the surface the changes look good (positivity), tests are passing but testing coverage was affected a bit. (circumstance) I guess you forgot to test the new mapping functions. Also, there are 3 concerning code quality issues raised by Sonar. (guidance/openness) Can you please have a look?”

Look for the main change

Find the file or files that are the “main” part of the change. Often, there is one file that has the largest number of logical changes, and it's the major piece of the work done. Look at these major parts first. This helps give context to all of the smaller parts, and generally accelerates doing the code review.

Identify any major problem

If you see some major design problems with this part of the code, you should send those comments immediately and reject the changes, even if you have time to review the rest of the changes right now.

Comment Etiquette

• Don't assume the obvious
• Ask questions
• Be nice kind
• Be courteous and respectful
• Accept different points of view

Request Changes ⚠️

“I believe this function is not typesafe and this might raise some problems mapping mixed numeric types. Plus, it doesn't check for empty values. I've noticed already that the numeric property `total` already lost the desired precision, and unit tests show that if the value if `tax` is absent the default value will be null. This can raise some problems in the upstream services. (circumstance) Leaving here an example on how can we possibly overcome this, let me know your thoughts:” (guidance/teamwork/openness)

						
							// BalanceMapper.java
							var taxMapper = new NumberMapper<BigDecimal> { (...) }
							var transactionsMapper = new NumberMapper<Long> { (...) }
							// Mapper.java
							(...).tax(taxMapper.map(tax)).transactions(transactionsMapper.map(transactions))(...)
						
					

Request Changes ⚠️

“This explicit constructor can be avoided, plus this class can be immutable. (circumstance) Please consider similar use cases in this package, where the Builder pattern is being used. (guidance) I believe it's an improvement that brings value without too much effort. (awareness) Please let me know what do you think and feel free to suggest an alternative approach.”(teamwork/openness)

Request Changes ⚠️

“The stubbing of these 3 methods in this test class are using generic values, this does not guarantee that the correct datatypes conversion is happening (circumstance), right? Please let me know if there's anything I'm missing.”(teamwork/openness)

Review logical sequence

Find closure

All questions raised are closed by the reviewers, and there's mutual agreement between both parts on the changes performed.

Code Review is approved

The necessary number of reviewers accepted the changes proposed