Pull requests and Code reviews

What is bad code?

Reference: Quora

How would you identify that you are writing bad code or tell someone that they are writing bad code?

You don’t because it isn’t bad.

It might be

1. logically flawed
2. stylistically at odds with team standards
3. not performant
4. inelegant
5. too verbose
6. too terse
7. not readable
8. poorly named
9. fragile

It might

1. follow anti-patterns
2. be untestable or contrary to architectural standards
3. violate language idioms
4. break tests
5. introduce regressions
6. confuse team-mates
7. poorly documented

It might

1. recreate existing library functions
2. abuse third party APIs
3. cost far too much to run
4. use language features which are already at end of life

It might

1. create tech debt~ and
2. make iteration more difficult

Their code might start a revolution.

Tell them any of that or whatever else might be true of their code but don’t just say it’s bad. Doing so shows a lack of technical depth and imagination. Do better by your colleagues.

At what point does “constructive” criticism of your code become unhelpful?

1. When it is too late: “I would use a different design”
2. When it is pure nit picking: “I would have called ‘create’ ‘realise’
3. When there is a deadline and we can accept the tech debt
4. When it results from inter personal conflicts
5. When it is based on a misunderstanding of the subject and constraints

This is a checklist that could be helpful when submitting and dealing with pull requests:

Before writing code

1. Pull from master/develop - whichever one is supposed to be used according to team norms.
2. Run the linters and unit tests before starting to make any changes - to make sure that everything looks good before you start making changes locally. yarn lint and yarn test.
3. Do not forget the usefulness of comparing the local branch with main (or whatever branch you are going to create a pull request against). Figure out how to do this using various tools like Eclipse, git terminal, Visual Studio Code, emacs, etc.

While writing code

1. Is there common code that can be put in a library as opposed to including it in the application itself?
2. Wherever possible, if things need to be added to constructors, put them in constructors - I’d ask myself: Should I re-use the same DependentService instance for all requests from the CurrentClass? Or do I need a new one for each request?
3. Passing parameters to constructors is a good idea instead of passing parameters to individual functions. Especially, if the methods use state and if they are not pure.
4. Make sure each layer like controller layer, service layer, etc. is doing what it is expected to do. Pay attention to separation of concerns. Things like validation should not seep into the deeper layers of the application. They should be handled as early as possible. Preferably, in the controller layer or in the service layer.
5. Wherever necessary, redact the sensitive data like passwords, tokens from printing on the logs.
6. In javascript, verify the use of single quotes can be replaced by backticks.
7. Go through the javadocs, jsdocs or descriptions for each single method and test case and verify if they are describing the method or test case appropriately. When method signatures are modified, make sure that the corresponding documentation on each method, test, etc. are also updated.
8. Check if there are any Bad programming practices

Overview of the changes

1. While working on a feature, write about the major changes in a text file within the application.
   1. Obviously, this shouldn’t be checked into source control.
2. This will come in handy when submitting the changes for review.

Testing

1. Do not put process variables or write code for process variables in test classes. Instead, mock them.
2. In test cases, if there is a chance to use tables, use tables instead of writing individual test cases. e.g. it.each with jest. This is also possible with junit.
3. Test API locally for end-to-end integration.
4. Make changes and test everything again - unit tests, integration tests, linters, etc.
5. Do you need to run regression and performance tests before merging these changes into the develop/master branch?
6. For javascript tests, make sure that there is a class level description at the top.

After writing code

1. Push your changes.
2. Deploy your branch to develop to make sure it works as expected in an environment other than local computer.
3. If there is a new service integration, is the documentation updated? Update the README file if necessary. Are the manifest files updated? Alphabetical order for the lists in the manifest files.
4. For there are changes related to infrastructure configuration, running builds need to be mandatory.

Before creating the pull request

1. Check if the work in the pull request is in alignment with the description and acceptance criteria in the story.
   1. If they are not, update the Jira ticket with the description about the work that is being done on the task.
   2. If the scope or the direction of the task changes mid-way through the implementation process, make sure that the Jira ticket is updated.
   3. This will serve as good documentation and help avoid having to answer multiple people on multiple occasions.
   4. This will prevent having to explain each and every change to each and every person reviewing the pull request.
   5. For anyone looking to find out why a change was made, the Jira ticket will have all the answers.
2. Check the CICD pipelines.
   1. Wait till the pipelines have successful builds before adding people as reviewers.
   2. Sometimes, there can be errors in with the pipelines and it is a good idea to clear them before asking people to review the changes.
3. Leverage Pull request checklists
   1. How to create: https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/creating-a-pull-request-template-for-your-repository
   2. Overview of the changes [Please describe overview of the changes here]
   3. Updated api specification files
   4. Regenerated the api specification index file
   5. Added database migration scripts
   6. Updated database model structs
   7. All the tests pass

Reviewing pull requests

1. For junior developers on the team, look at more than just the code before approving pull requests. Did they run the pipeline? Did it succeed? Did they update and deploy all the dependent applications?
2. Check if there are any Bad programming practices
3. See https://gist.github.com/katyhuff/845e06656f18784210190e4f46a4aa95

How to handle reviewer comments

1. Create pull request and send it out for review.
2. Fix comments for every single class and method before delivering.
3. You have to learn to not get emotionally attached to the code that you write. You have to deal with it objectively. If not, the comments on the pull requests are going to hurt you. There is a simple way to avoid this. Create pull requests and look at it from a third person’s perspective.
4. Think about the comments and things that are being proposed on the pull requests objectively. There is a pretty good chance that the suggestions sometimes make sense. If the proposals make sense, go ahead and make the change.
5. If the proposals do not make sense, do not leave wiggle room when responding to comments on pull requests. If we leave nice responses, some people who need a lot of attention will take advantage of the situation and they tend to write essays about code quality in the comments. Carefully write an explanation about why you think that the suggestion does not make sense and be firm about it. Sometimes, taking the conversation offline will help. This will give a chance for the two parties to sort out the differences and move on. Ultimately, we cannot get stuck with one single pull request. We have to move on.

Reading material

1. https://github.com/google/eng-practices
2. https://github.com/google/eng-practices/tree/master/review
3. https://howardism.org/Technical/Other/code-reviews.html
4. https://github.com/google/eng-practices/blob/master/review/developer/handling-comments.md