Nitpicks from a triggered senior developer in a code review

Courtesy: https://bit.ly/38cWpbZ

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?

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.

Take this interface for example
  • 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.

Clear, concise, documented behaviour.

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

Look at the order of the arguments. Do you notice anything?

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

The readability stems from the fact that the key function and filter “work” on a collection that is passed first

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.

This works. But ..?

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,

Proper ordering of annotations.

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.
How am I supposed to know what the “entity type” was for which no resolver was registered?
  • 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.
This was unintentional. However, a senior developer will catch it.
  • 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.
If the factory starts misbehaving on production, would we even know we are using the “default”

#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

Courtesy: https://bit.ly/38cWpbZ

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?

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.

Take this interface for example
  • 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.
Clear, concise, documented behaviour.
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

Look at the order of the arguments. Do you notice anything?

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

The readability stems from the fact that the key function and filter “work” on a collection that is passed first

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.

This works. But ..?

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,
Proper ordering of annotations.

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.
How am I supposed to know what the “entity type” was for which no resolver was registered?
  • 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.
This was unintentional. However, a senior developer will catch it.
  • 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.
If the factory starts misbehaving on production, would we even know we are using the “default”

#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


Print Share Comment Cite Upload Translate Updates
APA

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/

MLA
" » Nitpicks from a triggered senior developer in a code review." Aman Garg | Sciencx - Sunday March 7, 2021, https://www.scien.cx/2021/03/07/nitpicks-from-a-triggered-senior-developer-in-a-code-review/
HARVARD
Aman Garg | Sciencx Sunday March 7, 2021 » Nitpicks from a triggered senior developer in a code review., viewed ,<https://www.scien.cx/2021/03/07/nitpicks-from-a-triggered-senior-developer-in-a-code-review/>
VANCOUVER
Aman Garg | Sciencx - » Nitpicks from a triggered senior developer in a code review. [Internet]. [Accessed ]. Available from: https://www.scien.cx/2021/03/07/nitpicks-from-a-triggered-senior-developer-in-a-code-review/
CHICAGO
" » Nitpicks from a triggered senior developer in a code review." Aman Garg | Sciencx - Accessed . https://www.scien.cx/2021/03/07/nitpicks-from-a-triggered-senior-developer-in-a-code-review/
IEEE
" » Nitpicks from a triggered senior developer in a code review." Aman Garg | Sciencx [Online]. Available: https://www.scien.cx/2021/03/07/nitpicks-from-a-triggered-senior-developer-in-a-code-review/. [Accessed: ]
rf:citation
» Nitpicks from a triggered senior developer in a code review | Aman Garg | Sciencx | 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.

You must be logged in to translate posts. Please log in or register.