Code reviewing for the first time

In a previous post, I talked about my contribution to the IPC144 modernization project.

As a short TL;DR summary, I audited the pointers page, by fixing things like typographical errors, cleaning the generated Markdown syntax, and so on.

One of the other tasks I had to do was contribute as a code reviewer for at least two existing pull requests. The pull requests I worked on were Audit and Fix arrays.md, and Audited and Fixed portability.md.

Code review for arrays.md

The first code review, which was on the arrays.md page, was fairly short and straight to the point. Some details were already mentioned by previous comments in the thread, so I decided to focus on a different aspect of the page.

I mainly focused on the tables that looked like diagrams. For example, the following image displays a table used on the page, that was created by using some contrived HTML markup:

Thanks to the work of a previous PR, the primary maintainers of the new website decided that any similar tables may be transformed to images. The main advantage for that is to have a table that fits the viewport and that does not overflow; if there is no overflow, the user on a mobile view does not have scroll horizontally.

Code review for portability.md

This review was more thorough. I read the page several times, in the search of typographical errors, or something similar to that. Although I did not find a lot of typos, I did mention some improvements that could be done on the page, such as adding alternative text for images, and titles for links to improve SEO.

Lessons learned

Code reviewing is hard! Specially when you need to go through several details; it is very easy to skip one thing because you forgot about it.

Another thing that could happen is that you may spark some heavy discussion about a really specific thing (should we use this or that? Does this fit into the coding guidelines?). Of course, having a healthy discussion those help to advance the project in a more mature state: when you put certain ideas to the test, you may discover how strong or weak they are, and how we can either improve upon ideas, overall improving the whole project. The only issue is when one of the parties involved does not share the same ideal; they may view the review too confrontational, and may led to defending their way of doing the work.

At the end, a review should scrutinize the code so that it holds up to the standards of the project. If the project does prefer slightly verbose but self-explanatory code, then the author of the PR should respect this position and should adapt the changes to such standard.

Sometimes, some discussions may spark change in the standards of the project: the maintainers noticed the usage of macros helped readability of the code on certain cases, and thus will the project will move from a complete ban of macros to a slight usage of macros on really specific cases.

As long as both contributors and reviewers uphold the standards of the project, the project will naturally evolve, having better defined standards that future contributors and reviewers will have to uphold.