This content originally appeared on Level Up Coding - Medium and was authored by Aman Garg
We’ve all been there. For our pull requests (PRs), there’s this.
Hoping they safely make it through the barbaric eyes of a senior developer.
Hoping that we do not get nit picked on some “code” aspects by a pedant.
Hoping that we are only judged by the functional completeness of the task. Hoping that our 100 line diff will not garner more than a 1–2 comments.
The wishful thinking finally ends with this.
What gives?
In this article, we try to focus on small things that trigger a senior developer during a Code review. While the examples are more geared towards Java, I try to be language agnostic wherever possible. We’ll not focus on the language semantics or sonar smells, rather the soft aspects of readability and maintainability.
#1:Documenting the public API’s
Small interface. Big interface. Interface with only one implementation. Abstract class used only in one flow, two flows. etc. It doesn’t matter.
Always, document public APIs. This means, documenting the behaviours you expose in the interface. Documenting the methods designed for @Override in abstract classes.
Why?
Without this, there’s no way of knowing what the implementation is supposed to do and why it was designed that way.
- Just by reading this interface, I assume it’s a pure function since there’s no declared exception in the throws clause. Does it mean that the clients should handle / swallow exception if it arises? What kind of exceptions.
- What should the implementations look like? What all can they assume / not assume? What all is undefined behaviour?
- enrich() could definitely be a function that has a side effect that blows. Clients that solely rely on the method signature will be going places.
Instead, disambiguate this like below.
Public APIs are used more often than not. Interfaces will be extended by your peers in the future. Don’t do this to them. Don’t let the behaviour be a mystery.
#2: Ordering arguments in method signature
As long as my method receives all the arguments it needs to do its work, I’m good right? Not always.
We’re not talking about passing a small number of arguments in a method invocation. That’s not news. We’re talking about the order.
Take the below method for example. This tries to create a map on a collection using a key obtained from the keyFunction after running it through a filterFunction
The method arguments seem totally random. To obtain a map, a filter function has to be passed first. Why is that?
A client using this API will have the following workflow for a null filter.
collection c;
keyFunction k;
class.identityMap(..? should I pass null? I don't have a filter...)
By now, you would have realised that the method is most intuitively called by passing a collection (source of data), a grouping function (keyFunction) and then the filter(filterFunction)
So, choose this.
You first pass the collection and then the necessary arguments that operate on your collection. Small things like this improve readability
Why?
- Following this makes up for a more readable API that is a joy to work with. It’s also easy for the consumers of the API where the call parameters proceed in an intuitive order.
- Violate this and there’s a small cache miss (on the order) that every developer has to bear. Instead the arguments should flow like a stream merges into a river.
Detecting this subtlety requires experience. To an untrained eye, there’s no difference in the order. To embody this, think about the API usage at the clients.
#3 Ordering Annotations By Rarity For Decreased Cognitive Load
This by far, is one of my favourite nit picks. I also am amazed how many developers simply ignore this or not realize the gravity of it.
Rarer annotations on a member should be closer than the common ones.
An annotation is rare if it conveys a special meaning to the member it is attached to. If the same annotation is attached to multiple members, it is by definition, less rare.
Consider an API that requires a non null list of integers but they have to be within a range and the numbers have to be prime. In a JSR-303 specification, we would annotate the member like this.
Do you see a slight problem? If yes, congratulations.
NotNull and Getter are common. But there’s something off about the PrimeNumbersOnly and Range validation. Keeping a number in a range is an easier check than ensuring it is a prime (custom code).
Hence, PrimeNumbersOnly by definition is a rarer annotation than Range and needs to be closer to the member. NotNull is rarer than a Getter .
Instead, Look at below,
When I read the above code, my mind automatically switches gears and focusses more on the important annotations that serve the business use case.
Why?
- It’s a convention you can follow to make sure your codebase reads better to the devs. With practice, you’ll automatically start focussing on the most important annotations.
- Easier to read codebases and detect diffs in a pull request. Your eye automatically prioritises changes in the rarer annotations first.
#4 Log exceptions properly. Log assumptions. Log context
The violation of this is again commonplace. This is around logging.
- Consider logging extra helpful information around the original context that helps pinpoint the issue. This helps the on-call at 3 AM. Things like, what input was passed in the first place (if it was null). This also helps in easier grep-ing.
- Another is around the unintentional use of log.error in which developers fail to pass the stack trace information as a method parameter and pass it in the log string. This blows up later when a log is fired but there’s no stack trace to debug off of.
- Logging assumptions undertaken by a developer. This helps in debugging misbehaving flows where a certain context that was assumed to be true doesn’t stand anymore. Consider a sessionFactory that was silently initialised to a default value without any loggings.
#5 Adding redundant dependencies for fulfilling small use cases
I admit, I’m guilty of doing this in the past. Depending on javafx libraries just to be able to have a Pair<K, V> or a Tuple<A, B, C> only to find out later that the javafx classes are not included in OpenJDK and the build fails on CI.
Don’t do this. Roll out your own. How hard is it to write an immutable class that doesn’t add any unnecessary dependencies.
@AllArgsConstructor
@Getter
public static class Pair<K, V>{
private final K key;
private final V value;
}
A NodeJS equivalent would be to have a dependency on entire Lodash library just to be able to do a _.sum(array)
Unless you are convinced that the dependency offers a significant advantage, don’t bother adding it in. This is a red flag during the review.
That’s a wrap for this article. The points covered above, though nit picks, are important for a codebase’s health. With experience, you automatically start to favour readability. Hope this piece added value.
Nitpicks from a triggered senior developer in a code review was originally published in Level Up Coding on Medium, where people are continuing the conversation by highlighting and responding to this story.
This content originally appeared on Level Up Coding - Medium and was authored by Aman Garg
Aman Garg | Sciencx (2021-03-07T02:29:50+00:00) Nitpicks from a triggered senior developer in a code review. Retrieved from https://www.scien.cx/2021/03/07/nitpicks-from-a-triggered-senior-developer-in-a-code-review/
Please log in to upload a file.
There are no updates yet.
Click the Upload button above to add an update.