A wise man once said doing code reviews is a skill like writing code
I agree.
Like in the beginning, you need training wheels to get comfortable riding a bicycle. Then, eventually, you do not need them anymore. It's the same with coding, and code reviews. To get better at something, you need to practice it frequently.
In the last few days, I got to do code reviews (for the first time in my life) in My Photohub - A fantastic project being put together by a great team of developers.
My Photohub is a web app that makes it easier to share your photos on the web. You can read more about it here.
I also did a code review in vs-code-seneca-college extension in development, which aims to make collaboration for developers at Seneca College easier.
Review #1
Comment for
#10
@batunpc I was able to test your PR locally.
Your demo is looking good.
I noticed that you are not masking the input field for GitHub Token as password. It should be fine for now, but later on we should mask the value user enters for PAT tokens since GitHub recommends to treat tokens like passwords.
Lastly, there are some unused dependencies in the package.json file.
Good work! 👍🏼
My Reasoning
GitHub PAT tokens are like passwords, so they should be masked. However, since My PhotoHub is in its infancy, it helps make debugging easier if, as developers, we can see what we are entering as we type. Of course, we would mask the input before shipping the product.
Method of Testing
- Checking out the PR locally, using
git - Using depscan - a tool that scans for unused dependencies in a project
View the PR here
Review #2
My Reasoning
If we add more workers down the road, we need to modify the CONTRIBUTING.md file to add a publish script for each worker we introduce. By making the script generic, we are crossing out one extra commit per new worker.
View the PR here
Review #3
My Reasoning
The content must be free of grammar errors, for sure. However, you also need to ensure the document meets accessibility guidelines. According to an article published on the Yale University website, when using a sectioning element, a heading should generally be the first content within the section to act as a label.
Content preserves the reputation, but presentation preserves the audience count. Both are equally important.
Method of Testing
markdown-viewer - it beautifies markdown, but it also shows the output
View the PR here
Learning Outcomes
I learned that I need to understand the code to review it.
Also, I need to weigh the pros and cons of the changes that the pull request will introduce in the codebase upon merge.
Finally, I should checkout the pull request for projects that have a UI locally and be able to test it.
Top comments (5)
Wow, how did you add a comment from Github into the post?
Using embeds. You can read more about them here.
Thanks! Love it!
zxvdsDSDGVSDGF
v