Znalezione obrazy dla zapytania code review
source: https://spzone-simpleprogrammer.netdna-ssl.com/wp-content/uploads/2010/06/wtfs_per_minute_thumb.jpg

What I look for in a code review

Code review is important we all know that. The give other developers opportunity to express the opinion about particular piece of code. In recent years event the code review became the part of the merging request (or pull request). And particular functionality will not be checked in until one or more developers will give their stamp of approval.

Popularity of code reviews is a good think. The rising level of complexity of reality that your code is supposed automated leave the enormous space for abuse. Companies starved for developers hire students or boot camps survivors in greater amounts. I have nothing against these people I myself was inexperienced one and I met on my way some people who showed me a better way. But they often check a piece of code that I think this cartoon describes best:

Code review gives us a last opportunity to intervene. We should be like gate keepers standing guard to the treasure and filtering all the impurities leaving only gold. I the following list will be spitted in two parts. First part will be for the code that functions around our core domain. This is less important stuff that we should let juniors play with so they can learn. And there is a code for the domain, this is the stuff that either makes us money or prevents us from losing money. The set of rules for the first one will be more basic for the second one more strict and complicated. To borrow some terminology from DDD (Domain Driven Design) every code review that messes with the internals of our bounded context is subjected to much more scrutiny.

As a side note there are two types of commits we make to repo either to refactor code or with some new functionality. The rules that are bellow apply only to new functionality. When there is a commit with that refactor existing code I loo at it with different set of eyes. If I see a commit that combines the two types I politely ask the developer to roll it back and make two separate review requests.

All the things not in a domain

Clear is better than clever (aka: KISS)

This one is my kryptonite. KISS is an acronym for “Keep it simple, stupid". coined by U.S. Navy states that most systems works well if they are kept simple. Any signs of over engineering are like cat nip for my sarcastic comments. The code is a description of a certain task. And the intention and the logic behind it should be immediately apparent to the reader. It is worth mentioning that the code is written once but read many times so we should optimize for the second.

The problem is that writing clever code feels good. It’s like we are the Leonardo da Vinci painting Mona Lisa. That’s why we have code review :) the knowledge that as soon as I’m done someone will take a look carbs my enthusiasm.

Noticing cleaver code is easy. If you can’t understand it because it uses some construct from the language that is not meant to be used that way you know it is a code smell. Good example from the stackoverflow: you have two optional and you have to add them together empty optional is treated as zero, unless both are empty then return empty optional. Possible solution:

Optional<BigDecimal> add(Optional<BigDecimal> first, Optional<BigDecimal> second){
    return first.map(value -> second.map(value::add).orElse(value))
.map(Optional::of)
.orElse(second);
}

Another possible solution:

Optional<BigDecimal> add(Optional<BigDecimal> first, Optional<BigDecimal> second){
    if(!first.isPresent() && !second.isPresent()){
return empty();
} else {
return of(first.orElse(ZERO).add(second.orElse(ZERO)));
}
}

You can tell me which one conveys the intention better. So creating optional only so you can chain methods together is good example. I think developers will always feel the need to write the code so only God will be able to understand. And they need to get it out of their system. Organizing a code golfing contents in your team, might help :) or you can write a blog like me.

Don’t reinvent the wheel

Znalezione obrazy dla zapytania Reinventing the wheel
source: https://cdn-images-1.medium.com/max/640/1*ZmLFM5MP17SLa8fgXmaFdQ.jpeg

There are two sub points to that paragraph first

  • Venkat Subramaniam in his presentation (Core design principles for software developers) called it a primitive obsession. The desire to write at the lowest possible level. This is often driven by performance optimizations.
  • Writing code that has been implemented in a well tested library.

Developers are smart people but the are never as smart as they think they are. Driven by conviction that they can do a better job than someone else they can re-implement the for loop :). Comment: “Premature optimization is a root of all evil”

Here the code review serve a dual purpose as guy/gal might not know about the library XYZ that does ABC. So we can politely tell them to replace some peace of functionality with the library call.

A little copying is better than a little dependency

Let me start this paragraph with a quote

Duplication is far cheaper than the wrong abstraction

I think far too often we go in to this pattern see duplication-> wrap it in function, api call, library (thus creating a dependency) . Missing the point entirely on why abstraction exists. We shouldn't abstract the code to remove duplication or make the code “reusable” we do it to gain clarity into some businesses functionality. And to make it easier to be used by others.

I often see this. I’m taking away some functionality I’m still not sure how it is going to turn out but I’m refactoring furiously to define clear API surface area and remove duplication. That is a wrong path first if the code has one, two callers it is not an API it’s just a code that is being called from two places. Second during development it is OK for things to wobble a little bit, even a lot.

And we should always keep in mind it is easy to remove duplicates by creating a service centralizing that functionality but it might be extremely hard to do the reverse.

DRY (Don’t repeat yourself)

The definition of rule itself states:

Every piece of knowledge must have a single, unambiguous, authoritative representation within a system.

A little bit of a contradiction to the previous point. But let me explain. If I notice the code duplication in the same class it’s immediate code smell. We should leave the command ‘extract to separate method’. If the code duplication occurs in two classes this is somewhat more difficult to figure out. For me it is always a question how much business sense would it make to extract that to separate class and create a dependency in place of duplication. I know I’m repeating my self. And the irony is not lost upon me. but i’m writing code for 12 years and i’m finally starting to get where the line is.

Any code that executes the same business logic should follow quoted above and be in a single place. If the code is there for technical reasons I usually don’t bother as long as it’s just one line and exist only in two places. If that constraint is exceeded I usually start to thing of refactoring.

I recommend to have static code analysis that detects duplicate as it can often be missed by a reviewer. And in order to make a decision whether or not to remove depends on you knowing about it.

There is also another aspect to this that I don’t think is often mention. There are phenomenal tools for code generation. Project Lombok in the JAVA ecosystem is good example. With it we can use annotation to generate constructors, getters, setters etc.. We should take advantage of that. The best code is the one that has never been written. Some good annotaions:

@Data @Value @Builder @NoArgsConstructor @RequiredArgsConstructor @AllArgsConstructor @EqualsAndHashCode @ToString @NonNull

High cohesion, Low coupling

We write a cohesive code of similar things are together and unlike things are apart. Good example of that that I’ve seen many times is part of the business logic is written in SQL and part in JAVA. This lead to so many problems that I think I could make a separate article out of this. I will quote again:

Every piece of knowledge must have a single, unambiguous, authoritative representation within a system.

It is so important to follow that. The different peace of software have a different rate of change and change of the business rules should not trigger a deployment on the DB. Not to mention how hard it is to track bugs, debug issues, unit test any functionality that span across two, or more systems (God forbid that you also have some logic in the GUI)

I already complained a lot on dependencies in this article. And having a code that has high coupling is exactly that. Despite what you might thing the code:

@Autowired
AbcService abcService;

Is still quite tightly coupled. Yes it is true that you don’t depend on exact implementation of the AbcService but you still have to have it at compile time. That’s why i like to consider different form of Inversion of Control. And in order they are:

  • Dependency Injection
  • Events
  • Aspects

And try to go as loos I possibly can without losing clarity. As the lower on the list I go the harder it is to follow the code execution path. But I think that it’s good be reminded that there are other options.

Long methods

What exactly do I mean long methods. When do I leave a comment “this is a long method. Split it”. Is it 10 lines, 5 lines? I don’t think setting the number is helpful here. As it is not definitive. Bellow there are examples of two methods both doing exactly the same thing and one is 5 lines long another is 3 lines long. Can you tell me if one of this should be consider long?

private int sumAllEvenNumbers( @NotNull  int[] numbers){
int counter = 0;
for(int i = 0; i< numbers.length; i++)
if(numbers[i] % 2 == 0)
counter = counter + numbers[i];
return counter;
}

private int sumAllEvenNumbers2(@NotNull int[] numbers) {
return stream(numbers)
.filter( number -> number % 2 == 0)
.sum();
}

For me the first methods is to long and not because it consist of 5 lines but it operates on too many levels of abstraction. We have body of a method that is one, we have a body of a for loop that two and we have a body of if statement that is three. It is easy to spot it just count the number of indentation in your method. My spiritual guide( Venkat Subramaniam) called it SLAP (Single Layer of Abstraction). This is one of the reason why I prefer the functional style of programming. The clarity of what the second function is doing is apparent at first glance.

There is many reasons to avoid long methods. Event in experimental stages of the project. It is super hard to load them to our brain, for analysis. It is hard to test them, and they have a nagging tendency to become longer and longer.

Variable, Methods, Class names

There are only two hard things in Computer Science: cache invalidation and naming things.
— Phil Karlton

One of my favorite quotes. I will not go into much details here. We all know that code should read like prose. And good meaningful names is important part of that.

One code smell that i decide to add after some time is don’t include the type in the variable names. So naming array of integers arrayOfIntegers is a bad practices. Even naming the same variable arrayOfPriceses is something that should be pointed out. I think calling it prices is enough.

Also before the project you should have some sort of meeting where you will agree on the styling either camel case or _ . And I use code review to enforce it.

Comments

I’m not against comments per say. I’m against them if they are used to make up for bad code (comments like: returning array, declaring variable). Vankat has a good quote for this:

Comment why, not what

So comment: “Using BigDecimal type here because of extra precision required” is OK.

Also we should be extra sensitive for comments that have lost is’s meaning. Because the code has changes so much. So when I do a code review I always read a comment and think for a second: “Is this still valid”.

Favor composition/delegation over inheritance

Doing inheritance is hard. I will focus more on this subject when disusing code smells in the domain. Generally for the stuff outside domain the inheritance should be avoided.

On of the more popular smells that I see is extending DTOs. For me this immediately triggers comment: “Don’t use inheritance to share data it should be used to share behavior”.

Another great tip is to use @Delegate annotation from the project lombok.

public class Mother {
@Delegate Human human = new Humnan();
}

This will make all the methods accessible from the mother instance. Then you don’t have to maintain the invocation of the composition methods and therefore you don’t violate the DRY principle.

State and State Mutation

Again many things have been said about this. Generally it is well established knowledge that state mutation leads to problems (not talking only about the parallelism). That’s why object should either expose fields and they should be public final, or behaviors and all his members should be private.

During code review I look for getters, setters (violators of encapsulation) and ask in comment “is this necessary”.

I look for the types that are Mutable like Data, Collection objects in java, and I encourage to replace them with Immutable counterparts.

All things in the domain

So all the things i mentioned above are valid also for the code that is in the domain. But because this is the part that really matters there are some additional things that i’m looking for.

Unit tests

I was tempted to include that in the previous points but the truth is that it is not always required to have unit test over application that has little to no complexity. After all things that are not part of your business domain should contain that much logic. They are usually simple CRUD apps that’s much better write integration test for.

Different story for the domain stuff here the breaking change might be much more costly for your company so the unit test are a must. They force the user not only to prove the code is working but also force to think from the point of the user if this and that makes sense.

Violation of Single Responsibility

Finally. Next I will discuss the five SOLID principles. It has been said a lot over the years about them so briefly.

Single responsibility meaning this function, class does one thing and does it well. Looking at the code the primary responsibility should be apparent. Long methods, long classes are your enemies here.

Robert Martin defines it as “single reason to change”.

How I usually look for violation of this rule. Well (aside for what i said about long methods), too many dependencies:

  • more than 3 arguments in constructors (DTOs are exception)
  • more than 3 arguments passed to the function
  • more than 3 injection in a class
  • more than 3 invocation of other functions from a single method

Of to many tabs/spaces in a particular piece of code. (remember SLAP)

Violation of Open/Closed Principle

This is a difficult one. It involves the use of inheritance which i never recommend. So for me the first step should be to think if the composition won’t serve us better.

You see to take a full advantage of the OC Principle we have to know the direction of change. For example let’s say you have following hierarchy:

Payment 
-> CardPayment
-> pay(Amount amount)
-> CashPayment
-> pay(Amount amount)
-> CheckPayemnt
-> pay(Amount amount)

And you have some service that simply callers Payment.pay(). Now let’s say your manager comes to you and said that you have 3 day to add new type of payment it is exchange where the customers will leave 3 old toasters and get one new toaster. It will not fit in to your current hierarchy model and so you will probably and if statement in you Payment Service. Thus violating the OC principle.

During the code review I not only look for violation of this rule. Generally if you follow the best practices (apply KISS, DRY, SLAP) it almost never happened. Most of the time I’m focused on whether or not the inheritance is needed here at all.

Also I’m very careful if i stumble upon the usage of the world instanceof.

You see in order to use inheritance as a tool for openness for change we have to know the direction of changes in our organization. Meaning: has the domain expert been consulted on the matter. Has she/he confirmed that we should be open for change in this area.

Violation of Liskov substitution principle

Together with the OC principle are the reason why inheritance is hard to implement correctly. The rule states:

functions that use pointers to base classes must be able to use objects of derived classes without knowing it

Think about this for a moment. I like to explain it in a plain English:

Services of a derived class should require no more and promise no less than the corresponding services of a base class.

Still not clear enough, take a look at the code below.

@Override
public String someInheritedMethod(){
throw new NotYetImplementedException("I don't need this");
}

If you extend some class because you need only part of it’s contract you violate the rule. In this case delegation is probably a better fit. The problem with inheritance is far to easy to use and therefore far to easy to abuse.

Java is enforcing in some cases the following of that rule for example:

  • you can’t reduce the visibility of the inherited method.
  • you can’t throw new checked exception from derived class (unless exception extends one of the base exceptions).
  • Collection of derived does not extends from the collection of base.

Violation of Interface Segregation principle

This is a narrowing of high cohesion rule but for the interfaces. I always look for fat interfaces and ask in the comments to split them. As example I will user interface

public interface Human {
Integer heartRate();
Integer bloodPressure();

Integer brainWaveActivity();
Integer stressLevel();
}

Don’t you think don’t you think that it exposes too much functionality. Given the fact that regular first contact doctor will not care about the brain wave activity. I think following design is much nicer:

public interface BasicVitalSigns {
Integer heartRate();
Integer bloodPressure();
}

public interface BrainActivity {
Integer brainWaveActivity();
Integer stressLevel();
}

public interface Patient extends BasicVitalSigns, BrainActivity {}

Violation of Dependency Inversion Principle

The rule states:

High-level modules should not depend on low-level modules. Both should depend on abstractions.
Abstractions should not depend on details. Details should depend on abstractions.

So in plain English: class should not depend on another class. And this is again to be used with cushion as it can be a huge complexity buster if follow blindly.

I think the quality and coverage of tests is a good indicator if this rule is violated. Generally if the code is not well tested or you have to use one of the framework that allow you to mock the static final private methods you probably violate this rule somewhere. That is why I like Mockito as it allows you to mock only what should be mockable.

As a rule you should depend on as general abstraction as you possibly can. So the PaymentService should not depend on the MasterClardPaymentProcessor or the BitCoinPaymentProcessor it should depend on the PaymentProcessor interface that can be implemented by different methods of payment.

Summary

OK so this are the things I look in a code review. I don’t think that list is complete as every engineering problem is different. Some of us will write big server application for billion dollar companies and some of us will write audio codec or machine learning algorithms. And the requirements will set the tone. If CPU performance is our end gol we will have to compromise on readability in certain places. If extensibility is our gol focus will lie elsewhere. And sometimes we have to throw together something that just have to behave certain way so your company won’t go bankrupt.

So in reality all I can ask of you is to write code with humanity, clarity and simplicity in mind.