Reviewing pull requests from junior developers
Any respectable workplace and engineering team will have some form of code review. The idea is that writing code is both an art and science, and it is essentially a very human process. Like any other human process, it is possible that a person might make mistakes. Having another pair of eyes helps fix these problems.
There are many reasons why we need a code review process. Here are some of the most important ones:
Code reviews can help to find errors that the author may have missed, such as logic errors, typos, and security vulnerabilities.
Code reviews can help to make the code more readable and understandable, which can make it easier to maintain and update in the future.
Code reviews can help to make sure that the code is implemented correctly and that it meets the needs of the project.
Code review is a great way to learn from others. More experienced coders often help identify issues earlier and save a lot of time but they also help junior developers learn the art better.
Code reviews can help to build trust and collaboration among team members. By working together to review code, we can learn to trust each other's work and build a stronger team.
It is important when you work with junior code developers you should not just focus on the code under review but also the impact it has on the individual's learning as well as on the code quality.
Here is a typical checklist I recommend
Often code review requests do not contain enough description of why it was created. It is an important habit and it is an art. Junior developers should learn how to describe a change and document it properly.
Different companies have different processes and formats but it is a good idea to enforce a specific format on the PR description to achieve this.
Some of the rules could be
- Mention the ticket/bug id related to this change.
- Describe the change.
- Describe if the change has a feature flag involved.
- Describe what kind of testing was done on the code.
The ideal size of a pull request is a matter of debate, but most experts agree that it should be as small as possible. A good rule of thumb is to keep it to 250 lines of code or less. This makes it easier for reviewers to understand the changes and identify potential problems.
There are several reasons why smaller pull requests are better. First, they are easier to review. A reviewer can quickly scan a small pull request and identify any major changes. This makes it more likely that the pull request will be approved on the first attempt.
Second, smaller pull requests are less likely to introduce bugs. When you make a lot of changes to the code at once, it is more likely that you will introduce errors. By breaking down your work into smaller pull requests, you can reduce the risk of introducing bugs.
Third, smaller pull requests are easier to deploy. When you deploy a large pull request, it can be difficult to track down the source of any problems that occur. By deploying smaller pull requests, you can make it easier to identify and fix problems.
Of course, there are some cases where it may be necessary to create a larger pull request. For example, if you are making a major change to the codebase, it may be more efficient to do it in one pull request. However, in general, it is best to keep pull requests as small as possible.
Asking developers to make pull requests smaller makes them think about the system design better.
In your feedback on the pull request always offer very constructive feedback. The thumburles of constructive feedback are the following:
- Always be specific and actionable.
Any comment on a PR should be very specific. For example
Variable on the line number 22 can be a string constant defined in a separate file. This will improve the design because this constant is likely to be used by other methods in future.
Why is this a variable and not a constant ?
Note that in the good feedback, you point out exactly what you are talking about and how to fix it and why to fix it.
Remember that the person who submitted the pull request is a human being. It is okay to disagree with their approach, but it is important to be respectful and constructive in your feedback. As a senior developer, you have a position of power, and misusing it can be harmful. Many developers are motivated by encouragement and positive feedback.
It is not just about the specific pull request. It is also about creating a culture of openness, mentorship, and commitment to quality engineering. When we treat each other with respect and support each other's growth, we create a more positive and productive environment for everyone.
From what I can see, I think this code might not be doing what you have claimed in the PR description. Could you please write a unit test to check if this would work if the input array has negative numbers in it ?
This is perhaps the worst way anyone can create a sum of items in an array, couldn't you put even basic thought into this ?
Always ensure that the person has gotten your feedback and has worked on it to fix those issues. If the person is a junior developer it is totally fine to walk upto them and explain to them any finer aspects of their code and offer to help them.
Junior developers often make common mistakes. One way to quickly resolve these mistakes is to create a cheat sheet of comments that you can copy and paste for each mistake. Over time, you will find that certain language and terminology makes things easier to explain, and having a template library of these comments will be very helpful.
Coding is a collaborative process and you will often learn many things from others. So during entire process keep an open mind to learn from others including junior developers.
Since this is blog about front end engineering there are some front end engineering aspects of code review that I want to highlight.
Always make sure that the proposed change does not have an impact on web performance. Ask the developer to use tools like Lighthouse to test it out.
Accessibility is often overlooked by developers, who may lack the necessary knowledge or awareness.
Many developers are not familiar with accessibility best practices, which can lead to inaccessible websites and applications.
Accessibility is a complex topic, and it can be difficult for developers to keep up with the latest standards and guidelines. Even developers who are knowledgeable about accessibility may not always prioritize it, especially when deadlines are tight.
If it matters for your project make sure the code does not impact SEO negatively. Any introduction of new semantic tags, new content, links, dynamic content can have an impact on SEO.
Ensure the developer has followed the best practices laid down by your company.
It is important we learn to get better and help others get better. When reviewing code always keep in mind this principle. Educate your juniors to get better and this can often be done by automating most parts of your code review process.
This day and age, we have access to superior code generation tools as well as static analysis tools to fix a lot of common problems.
As a senior developer you should work to make those changes so that lot of issues get caught by a machine and not human.
When reviewing code by junior developers focus on being respectful and always offer specific and actionable feedback. A code review comment would typically education on why you are offering the feedback and often form it as a question rather than a statement. Also always be prepared to help the developer if they are struggling.