Babbel Bytes

Insights from the Babbel engineering team

Juniors' Guide to Pull Requests

Aimee Nortje –

Guidelines on confidently reviewing senior team members’ code without feeling like a total newb.


Sitting down at your desk in the morning and the first thing you see is the pull request from a senior that has been sitting there waiting for you to review.

“No reviewers approved yet. Why am I even added as a reviewer? I don’t know what I’m doing. She (who created said pull request) obviously does. She has been here so long and practically wrote the entire codebase. I’ll just wait until somebody else approves it first.”

We’ve all been there, right? The daunting feeling of ‘this isn’t my place to comment’. What if the pull request gets merged and there’s a bug and it affects hundreds or even thousands (millions?) of users, and my name is on the list of approvers.

As a junior I definitely experienced that internal conflict, but at some point in my career, I realised there is always room to contribute. One little thing even. If you don’t feel comfortable to comment on or approve a pull request, you should be able to still learn from it. In my experience I would start by checking out the branch, change something, try to see what changed, debug, get a value I didn’t expect… hmmm… bug?

“Did I just find my first bug that’s not in my own code?”

Nope, perhaps I just didn’t understand what the code was meant to do. Why didn’t I understand what the code was meant to do though? Going deeper, I would come across a variable that is named, well, something incomprehensible. Ok, so at this point you should hopefully see where I’m heading with this without giving the play by play of my train of thought in looking at a (usually more senior) teammate’s code.

The thought of compiling a list of things to look at in pull requests was inspired by a previous company I worked at, but was more aimed at new joiners who didn’t have context on the codebase yet. As a more junior/inexperienced team member, there are still items you can view and confidently comment on, even if you feel too intimidated to actually approve a pull request. These are things that don’t necessarily require you to fully comprehend the surrounding context of the entire feature. Use them simply as a starting point, as a comfortable platform with which you can iniate comments, questions, or suggestions.

Some of the below points are self-explanatory, but it will hopefully give you some confidence to actually start looking at the code in order to learn, and in time, contribute. Note that the examples below are overly simplified for effect.

RM: Readability & Maintainability

F: Functionality

D: Design

Things to look out for

  1. Spelling mistakes or typos (RM)

  2. Agreed upon naming conventions (whether you’re following a style guide or just based on a team agreement) (RM)

  3. If the naming of functions and variables are descriptive and make sense (nothing like dom for dayOfMonth) (RM)

  4. Possible runtime errors (.filter on undefined in JavaScript for example) (F)

  5. Commented code that should be removed (RM)

  6. Adhering to style guides in terms of file/folder structure and naming convention (RM)

  7. Tests are included (RM)

  8. Do the names of the tests make sense (is it clear what is being tested)? (RM)

    The below is clearly a mistake in the name and we’re not testing here whether or not Berlin is in the database, but checking for Vienna.

    test('city database has Berlin', () => { expect(isCity('Vienna')).toBeTruthy(); });

  9. Formatting - in case there are no linting rules to auto-format / break builds (RM)

  10. DRY (Don’t Repeat Yourself) (D)

    Sometimes it’s easy enough to spot this when you see methods with some similar looking code, see if you can figure out if code can be added to a helper method for example, or simply ask if the developer can explain the reasoning behind the duplication (very often the answer is ‘Oh I forgot I did the same thing in the other place’, or ‘Yeah I was getting to that and then… well I didn’t’).

  11. Single responsibility (D)

    This may not always be so easy to spot as a junior, but it could, at least, prompt you to ask a question. A very simple example: If a function is aptly named getUsernameById, you could assume that you will be passing a user ID and getting a username in return. Does this function also update a different record with the user’s username? If so, big question mark.

  12. The code is in the correct place in the file structure (D)

    If it’s related to an Order, is it in the OrderService.

  13. Exception error messages are understandable (RM)

    Does the message actually describe an error and not just stating that an error occurred?

  14. If && and || are used correctly according to desired logic (F)

  15. Check that the relevant documentation has been updated/added (RM)

  16. Take one block of code, and dig into that and make sure you fully (like, fully) understand what that is meant to do. If you don’t understand what that is meant to do, add a comment to the pull request and ask for the creator of the pull request to explain.

  17. Ask questions! Be curious and ask the developer to explain why he/she did what they did, out of pure curiosity and learning about certain design decisions. This may either not have an impact on the outcome of the pull request, or it could potentially trigger a thought from the developer of potentially doing it in a different way when having to explain what their intention/reasoning was. Don’t be shy to ask the creator of the pull request to walk you through it offline.

  18. Check if the flow of the code makes sense. If you, even as a junior, can’t figure out what is going on, it may be a hint that the code has room for improvement.

What seniors can do to help from the start

  1. Ensure that you include a description of the changes you made. It is very useful to have an agreed upon template to include in pull requests, for example: 1. What did you change? 2. Why did you change it? 3. Notes and references.

  2. Encourage the junior to debug and see if they can follow the flow, even if not providing feedback on the pull request.

  3. A bit from the other side of things - when juniors create pull requests, ensure to leave comments to question certain things and make sure the juniors understand why they did what they did and not just because “it works”. Encourage them to also learn from these comments.

Sources

How to encourage junior developers to participate in code review

What to look for in a code review

Facebook Twitter Google+ Reddit EMail
Comments