3. Pull Requests & Code Review
At CodeDoesGood all features, bug fixes, and any other kinds of work to be merged into the project must be code reviewed and approved prior to merging.
Lead Mentors can choose how pull requests are handled. There are two options:
- Mentors may be responsible for approving pull requests. In this case one approval is needed.
- Hatchlings may be responsible for approving pull requests. In this case two approvals are needed with at least one of those approvals being done by a level 6 or 7 Hatchling or a developer/mentor level teammate.
What a Pull Request Should Contain
What to Look For When Reviewing a Pull Request
How to Give Feedback
How to GIVE Feedback
Because we have no quality department and no dedicated project managers it is very important that code is fully reviewed before merging. Below is a list of what your pull requests should contain and also what you should look for when reviewing your teammates' pull requests.
All developers on the project including Hatchlings of all levels should be able to ensure the following:
- Adherence to the style guide for the project.
- Commented out code should either not be present or should contain a TODO.
- Work arounds which are different solutions from what a developer on that platform would normally expect, large chunks of code taken from other sources, etc, should have comments explaining them as per the style guide instructions.
- Large functions can be broken down into smaller ones. This makes testing easier and it also makes understanding and reading the code easier.
- The feature is fully tested.
- All tests pass on both simulator and device.
- Run the project and ensure it works as expected.
When code reviewing other teammates' work, the above "Basic" list will ensure the code base remains clean and the integrity of the project is not compromised. In addition to that we also want to help each other learn. You can do this by watching for the following:
- Look for alternative solutions. If a developer has solved a problem using one method and you are aware of another way of doing it, leave a comment with that information. Do not be afraid to comment on an experienced Developer's code, the sign of a great developer is one that is always interested in learning and you would be surprised how much an experienced dev can learn from a new dev who has a different perspective!
- Help each other learn more advanced language techniques. For example in Swift nil coalescing can often be used in place of an if-let-else statement, help each other learn these things.
- Ask questions about code you do not understand and ask questions about code you do understand but do not know why the developer chose that particular solution.
A review isn't worth much if you don't truly review the code. You should pull and run the branch if there is anything complicated that you need to verify works as planned and also if there is a lot of UI work. If the pull request is submitted by a lower level Hatchling it is highly recommended to pull and run the branch and check the UI thoroughly including constraint handling (layout).
Here are some things to look for:
- Should a class keep a reference to a variable or should it have a local scope? (be created inside the function where it is used)
- Are there any retain cycles? Pay special attention to closures/blocks.
- Does a variable need to be mutable or can it be immutable?
- Are computed variables handled correctly? Are we ever creating a new instance each time it is accessed when we should not be / when the developer meant to only have an initial value?
- Is the dependency manager file set up correctly? Are versions specified in the dependency manager set up? Did the developer update a library when they should not have?
- Is there commented out code? (hint: There shouldn't be under normal circumstances)
- Should a complicated implementation have comments/documentation?
- Do assets have appropriate file names?
- Are files in the proper folders (both inside the project and on disk)?
- Are there empty functions? (for example in iOS an empty viewDidLoad added by the template file)
- In MVVM is proper separation between layers maintained?
- Are protocols, managers and classes named appropriately?
- Are long functions broken up (encapsulated) into smaller functions?
- Are related functions grouped together?
- Is formatting consistent?
- Are there tests for ever class and function added and are those tests thorough?
- Are there manager classes written for database and networking libraries?
Remember that the point of feedback is to learn and to teach. Avoid using language that can be interpreted as accusing someone. Read a comment back to yourself and ask yourself how you would feel if someone said this to you. Do your best to word your feedback in a way that is curious, compassionate, and helpful. Most importantly ask questions before making assumptions about the code you are reading. Often there is a legitimate reason that something was done a certain way and rather than telling someone they should change their work first ask them why they chose this particular solution.
Never tell someone they should change something or you do not think they have the best solution without providing some suggestions. If you have no idea how to fix the issue that you have spotted, tell them this in the comment and offer to work with them to find a better solution together.
When the feedback you need to give is more than just a simple change or a simple comment we encourage you to chat with the other person on slack or arrange a time to screen share. Especially for a newer developer often written explanations are difficult to follow and much time, confusion, and confidence can be saved by doing a demo via a screenshare.
Don't be afraid to use the #general-leadership channel to discuss this topic with other Mentors. Other Mentors can help you figure out how to explain certain topics and they will be an incredibly valuable resource; for example they may have tips if you have an especially sensitive Hatchling on your project.
We are all here to learn, every single one of us including the Developers, Assistant Mentors and the Lead Mentor on your project. Anyone who thinks they have nothing left to learn has no place at CodeDoesGood - that's just not an attitude we support.
Do. Not. Take. Feedback. To. Heart.
We can not stress this enough. We know that when you are learning your confidence is fragile and you will most definitely experience days where you don't feel good enough to be on the project, where you think you will never be a "good" developer, where you think you have chosen the wrong career. These kind of thoughts are a totally normal part of learning a new career and are especially common when learning to program.
Remember: YOUR CODE IS NOT YOU
There is nothing about your code, whether it's great code or truly bad (seriously, we all write bad code sometimes), which reflects who you are. Code is just letters on a screen. Learn to distance yourself from it and rather than wasting energy being upset about feedback realize this is an opportunity to learn. If you are distracted by being hurt or by talking down to yourself and calling yourself names, or by being angry at the person who gave you the feedback, you are missing out on a real opportunity to learn. A human who is emotional and absorbed with these kinds of thoughts simply can not learn and grow as well as one who is able to say "Oh, thanks so much! I didn't know that!"
No matter how feedback makes you feel remember that it was given with two intentions in mind:
- To teach YOU
- To maintain a quality codebase
If you are feeling especially sensitive or hurt about feedback you received reach out to a peer supporter, post in the #general-devhatchery channel on slack, talk to another Mentor. If you feel up to it talk to the Mentor that gave you the feedback. Let them know how their feedback made you feel and (this is important) let them know what they could have done differently in giving you that feedback. Everyone is different and our Mentors are working with many Hatchlings. There is no one single correct formula. Some Hatchlings will react in a sensitive way to feedback that other Hatchlings will not. If this happens to you help your Mentor learn the best way to give you feedback.
If you are a Mentor or Developer be aware other Mentors and Developers may give you feedback about the feedback you are giving others. Do not take this personally. If they tell you that they saw your comment to a Hatchling on your project and they suggest next time you say things differently remember that they are telling you this as a way to help both you and the Hatchlings on their project. The words that we say do not define who we are as a person. Remember this because if a Mentor gives you feedback and tries to help you change the way that you give feedback to others, all they are doing is trying to help you change the words that you say. It has absolutely nothing to do with who you are as a person. It is very important as a Mentor on the project that you are an example to the Hatchlings. Take feedback with grace and demonstrate good sportsmanship in your response.