Juan Diego Raimondi

Tags

This has been a common question and a subject of debate since everyone has their opinions on what good code is and what bad code is. Regardless, the pull request is not about the code itself but about the actions of reviewing, adjusting, discussing and assimilating (merging) code, which may be good or bad in itself.

This will be later followed by a set of notes on how to perform a good code review.

1. Before submitting

1.1. Decide on the goal for the pull request

Most of the times, pull requests are to have code merged. But not always. Sometimes, the objective of a pull request is to just have feedback on a particular aspect of design or implementation. Sometimes, it’s just to see if a particular feature will be accepted into the code you’re contributing to. Indicate the objective in the pull request. Once that objective is reached, make sure the code is merged or the PR is closed. Don’t leave unfinished PRs hanging around, since then what needs to be done with it is not clear.

I just need some feedback about the design of the XYZ class. It is not yet ready to merge.
Feature: preserving user data while offline.

1.2. Make sure it compiles

Needless to say, don’t submit code that does not compile. This could maybe be acceptable if the objective is to have some feedback about part of the code, but it goes beyond saying that the submitted code will not be in a state of being integrated. If the repository/tool to submit pull requests also run tests, make sure they pass. Otherwise, you’ll get a bunch of red lights or rejections for unnecessary reasons. Again, this can be an exception if you’re just looking for feedback of a particular aspect and not code merging.

This includes warnings if the approach in this project/team is not to allow them altogether. If that’s the case and there’s no way around it, you can always change the configuration of your tools to allow for exceptions to the rules. If you do that, make sure to document as closer to the exception as possible why this is unavoidable.

1.3. Make sure it can be merged to the destination branch

Resolve conflicts before creating the PR. Make the necessary integration changes before creating the PR. Reading through conflicts is confusing and will just generate more questions or demands from the side of the reviewers.

1.4. Make sure all tests pass

Having broken tests that will need to be fixed later will only create technical debt, or worse, hide real bugs, or stop the team from making progress if the test suite is broken. Fixing them while the PR is open will only generate more “back-and-forth”s with the reviewers, delaying the process and taking more time from everyone.

1.5. Make sure all aspects of the original requirement were addressed

Before submitting the PR, make sure it is complete with everything that you intended. If you have a requirement or project tracking system, review the tickets that the code was supposed to address. Review the design restrictions and make sure that the code being submitted complies to all of them. If you have a code style guide, make sure that the newly submitted code complies with the standards. If you have a UI style guide or branding guide, make sure UI changes conform to that before creating the PR.

Possible exceptions to this rule may be point 3.1. Do not include garbage, for when addressing all of these requirements lots of non-functional changes, or when the code itself is legacy or for some reason does not need to comply.

Another exception may be point 3.3. Break it into manageable chunks, if the requirement in itself is too big and can be broken down in pieces for better review, so you can address the particular refactors/UI changes in a separate PR.

2. Data about the pull request itself

2.1. Reference the tracking ticket you are using

If you are using a requirement / project tracking system, indicate which ticket this code is solving or contributing to. If there is none for this, either request the team to have one created or indicate that no ticket has been created for it. This will properly communicate that you went through the effort of looking through the requirements, and avoid duplicates in the future. When a team manages multiple PRs, being able to go back and forth between the code repository and the tracking system helps in determining the status of the project.

Some teams also prefer to have the distinction between the type of changes, like “bug”, “feature”, “refactor”, “improvement”, etc., visible in the PR. Find out what the approach is in this team and follow this convention too.

Fixed user permissions when saving local data [LS-102] Fixed user permissions when saving local data
[Several tickets] Fixed user permissions when saving local data [Feature] Preserving user data while offline
Preserving user data (fixes #5, #6, #7)(1)
[No ticket] Fixed favicon dimensions for modern browsers

(1) When the references to tickets are fixed in size (like the LS-102 example) I prefer them in the title. When they can vary a lot, (like the #5, #6, #7 example) I like them better in the body of the PR / commit. However, this is just personal preference. Follow whatever the team is doing.

2.2. Use a concise, yet descriptive title

Make it short, make it obvious. What did you do? Don’t explain the “why”, don’t explain the “what for”. In a title, explain the “what”.

Since most repository hosts also will take the name of the branch or the message of the first commit, this should also follow the same guidelines as commit messages: do not exceed 80 characters, make it simple, make it concise. Use the contents of the message for the description, not the title.

Adjusted permissions for saving local data for regular users (permissions: user-saving, localstorage-access) Adjusted user permissions when saving local data
Several fixes “My profile” UI design improvements
[LS-102] Users cannot save local data [LS-102] Adjusted user permissions for saving local data

2.3. Indicate changes and/or intentions

In the description of the pull request, elaborate a little bit on what you did, and the “what for”. Do not repeat the contents of the ticket you’re trying to address. That adds no value. Rather than that, explain (if necessary) what changes you made to achieve that goal.

It’s important to indicate which are the changes that you made, instead of which are the changes that you intended to make. Any bugfix or further refinement would have the same message if you just copied and pasted the final result as you intended in the first try. And having multiple commits or PRs with the same name does not help much for tracking.

**Title:** Adjusted permissions for saving local data for regular users (permissions: user-saving, localstorage-access) **Description:** (Empty) **Title:** Adjusted user permissions when saving local data **Description:** Regular users were not granted the following required permissions
• user-saving
• localstorage-access </ul> These permissions are now granted to all users, regardless of their access level.</td> </tr>
**Title:** Several fixes **Description:**
• Fixed favicon resolution for modern browsers
• Adjusted permissions so that users can save their own data offline
• Changed input types to HTML5 standard ones
• Changed colors </ul> </td>
**Title:** "My profile" UI design improvements **Description:**
• Fixed favicon resolution for modern browsers
• Changed input types to HTML5 standard ones
• Changed colors </ul> Note: The bug about users not being able to save local data will be included in a different PR, this one only addresses UI issues.</td> </tr> </tbody> </table> ### **2.4. Explain changes done** If the changes require some understanding or some design, or some non-obvious decisions were taking, document them here. They will make reviewers lives easier, and in consequence, yours too. Any investigation and thought-process you did that made you arrive at non-obvious conclusions also help here. Prevent reviewers from falling into the same problems that you had already faced and explain why the most obvious solutions were discarded. </tbody> </table> In my current team, we came up with the following template (just use it as an example): - **Background:** explain what the problem was that you were trying to solve anyway. Indicate any investigation that you did to find the original problem instead of the symptom that it usually is found when talking about bugs. - **Changes done:** explain any non-obvious changes that can be seen by reviewing the code. Why did you take a particular approach and not another? Why did you choose certain names for certain elements? Are you introducing new concepts to the system, and if so, which are those? - **Pending to be done:** Technical debt you decided to leave (and why). Bugs that were introduced already identified (and why you left them). Shortcomings. Things out of scope that you think may get you asked "why didn't you fix it?" Things you'd like done but you thought "today's not the day". - **Notes:** any miscellaneous stuff that reviewers should know about. (For example, is it better to review code in a certain way rather than looking at the diff linearly?) - **Reviewers:** who in particular you want to review your code. Maybe they're experts in the area you're working. Maybe they're the leaders that coordinate your group. Maybe it's someone that will be directly affected by it. ## 3. Content of the pull request ### 3.1. Do not include garbage I covered a little bit of this idea in my previous post, [Taming your tools](https://blog.alphasmanifesto.com/2016/02/28/taming-your-tools/). Every content of the pull request that is not helping to achieve the goal is helping to hide it. This usually goes against the tendency of opportunistic refactoring and adjusting styles. I personally prefer to leave them for subsequent PRs, but use your judgment. If your opportunistic refactor is a variable name change that affects only two lines, yeah, go ahead. If completely changes the design of an approach for greater flexibility, you may want to give it a PR of its own. Remember that the longer a pull request is, the less likely you're going to have it reviewed. Furthermore, if you're including a lot of changes, it is likely that people will miss changes that are more important when reviewing everything. One way to approach this is to make only the minimal-required changes for your approach to work. If you found refactors/improvements that you still want to make, you can branch them off and include those PRs later, or create a PR that goes against the branch of the first PR. Then the team may choose to approve those and merge them all together or merge them piece by piece in a reverse-cascade (more on this on point _3.3. Break it into manageable chunks_). The point is: have the changes being reviewed be the changes that you want reviewed. Don't mix them up or clutter them with unnecessary changes. Review each line before performing your PR, ask yourself: "Is this needed? Is this worth it to have in here?" ### 3.2. Stay focused on the original intention of the change This applies when deciding which changes should be part of the PR and when discussing feedback of the PR itself. Keep you mind focused on solving the original problem that you were trying to solve, and don't let it wander off too much. Don't include opportunistic fixes or "miscellaneous stuff". Not only it will make reviewing harder, but it will create difficulty in tracking changes in the future and it may actually delay your submission because those points will need to be discussed as well. You may be holding off merging an important feature or fix because you decided to also include a change to the wording in the UI. Importantly, decide on the scope of the changes that you're making. Sometimes a piece will require some other piece to be adjusted, and you don't want to start a cascade of changes thorough the whole code just to address a particular change. ### 3.3. Break it into manageable chunks The PR itself should be readable in a considerably short amount of time. You want your reviewer's full attention and if you're submitting way too many changes, it's very likely that won't happen. If the changes are just too many to make it a manageable PR, break it into chunks. You can always submit a module design even if that's not integrated with the other code. You can always submit changes to one class without including the changes to the others, as long as this does not break the system altogether. Think about the modularity of what you have done and take advantage of it. Depending on how your team manages this, they may want to review the whole feature before merging instead of merging it in chunks. If that's the case, create a branch on top of a branch and create nested PRs, like so: - branch1 has as destination main-branch -- this is PR 1 - branch2 has as destination branch1 -- this is PR 2 - branch3 has as destination branch2 -- this is PR 3 Once all three PRs had been accepted, you can then merge them in reverse order: PR 3, then PR 2, then PR 1. The set of changes will be the same that you had originally, but you allowed the team to review it in bite-size pieces. In order to keep this working, it is important each of the branches is coming out of the right place. See _4.2. Keep it updated to the destination branch_. ### 3.4. Missing pieces: make them obvious and explain why You may find parts that need to be fixed later but they are not relevant to the changes that you're making. Maybe a bug that came out of analyzing that portion of code. Maybe a possible risk that is not meant to be addressed right now. Maybe a refactor that will have to be done for readability. Maybe there are parts of the newly introduced code that are left to be completed or changes that need to be addressed in the future. Whatever it is, if you found it and decided not to make these changes right now, make it obvious to reviewers and future visitors of the code. I usually prefer to use //TODO comments, since most IDEs will recognize them as a set of pending tasks. Some others also recognize //FIXME and //HACK, with some of them even allowing to set priority levels for them. Regardless, make it obvious that there's a certain piece that you know it's missing, but it's not meant to be done between the changes that you're doing. When doing so, explain: - What change needs to be done and to achieve what purpose - Why the change cannot be done right now - If there's any tracking ticket associated to it, which is it
• localstorage-access </ul> These permissions are now granted to all users, regardless of their access level. It seems permissions were removed from all users in commit 12f3af3, to fix LS-94.</td> </tr>
**Title:** Adjusted user permissions when saving local data **Description:** Users did not have the required permissions. Now they have been given to them. **Title:** Adjusted user permissions when saving local data **Description:** Users did not have the required permissions. Now they have been given to them. Permissions are usually not granted at a global level. However, it is ok to give these permissions to a global level because all users should have access to these permissions, regardless of their access level. Non-registered users will not have these permissions applied because they do not get an instance of IUser until they create an account.
//TODO fix this //TODO refactor this -- code is unreadable (but not a priority right now)
//FIXME user permissions are wrong //TODO review given user permissions, they may be not allowing users of type "regularUser" to save data (LS-102)
//TODO rename this method to "saveUserData", as it explains better its purpose. //Won't do that right now because the API depends on it and that involves major changes. (LS-105)