Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

The Code Review Pyramid

morling.dev

Post reply on HN
Print view View original

21–30 of 117 posts

Re: The Code Review Pyramid

#21

Post by convolvatron »

mpalczewski wrote: post #8

First thing I review is readability. Code should be readable. Once code is readable it makes reviewing the rest much easier. Readable code is more maintainable and problems jump out at you. Stuff other than readability is important, but if you focus on readability it makes the rest of the review go very smoothly.

unfortunately, 'readability' is fairly subjective. I was part of a group recently that spent a good couple hours every week arguing whether 'ctxt' or 'context' was more readable.

when confronted, they explained to me as one would a child, that they cared deeply about code quality

Re: The Code Review Pyramid

#22

Post by _nhynes »

convolvatron wrote: post #21
mpalczewski wrote: post #8

First thing I review is readability. Code should be readable. Once code is readable it makes reviewing the rest much easier. Readable code is more maintainable and problems jump out at you. Stuff other than readability is important, but if you focus on readability it makes the rest of the review go very smoothly.

unfortunately, 'readability' is fairly subjective. I was part of a group recently that spent a good couple hours every week arguing whether 'ctxt' or 'context' was more readable. when confronted, they explained to me as one would a child, that they cared deeply about code quality

That's ridiculous.

Obviously `ctx` is the most readable.

Re: The Code Review Pyramid

#24

Post by maze-le »

_nhynes wrote: post #22
convolvatron wrote: post #21

Earlier quoted context omitted.

unfortunately, 'readability' is fairly subjective. I was part of a group recently that spent a good couple hours every week arguing whether 'ctxt' or 'context' was more readable. when confronted, they explained to me as one would a child, that they cared deeply about code quality

That's ridiculous. Obviously `ctx` is the most readable.

Its obvious from the context that "c" is the only variable name that should represent a context.

Re: The Code Review Pyramid

#25

Post by ilovecaching »

kolanos wrote: post #5
ilovecaching wrote: post #3

This doesn't seem correct to me. Each of these categories should bear equal weight, I won't spend less time on code style or tests to spend more time on API design. My code review checklist is a checklist. I spend as much time as necessary on each item in the checklist, no more no less. I also think that code style, fuzzing, and benchmark tests are the most important items on my checklist. It keeps the code base from…

Code style is the first thing I eliminate from a review process. I find the most commonly used code formatter and linter for $LANG (the more opinionated the better) and plug it into pre-commits and CI. Code style is just not worth my time.

You're conflating style with formatting. No formatter will tell you if you've picked clear names or need to decompose or combine something for better readability.

Re: The Code Review Pyramid

#26

Post by Cerium »

willcipriano wrote: post #18

The more experienced I get the more I want to work on teams with a higher degree of trust than ones like this. Don't get me wrong I think code review can be valuable but on high preforming teams it works best on a as needed basis. The way this has worked successfully for me in the past is as a new joiner to a team you submit for code review every time. My first couple of reviews tend to have a good bit of feedback as…

I work on a team that runs following a high trust and domain ownership model. When I joined our process was "write some code, test it, be confident in the code, commit it to trunk". A number of years ago we worked our way through "Clean Code" in a reading group and decided to try out code reviews. Each week one of us would present our recent work and the team would tear it apart line by line, and in the process we developed a better understanding of our group philosophy around style and practices.

When we switched to Git we decided that we liked code reviews and wanted to make them part of merging to develop. Depending on the content the reviews are deeper or more superficial. Many times even when I review the code of somebody making changes in their own subject matter domain I have found bugs that simply needed another set of eyes to catch.

I like code reviews and in the context I use them find that they are a complement to trust, not a substitute.

Re: The Code Review Pyramid

#27

Post by [deleted] »

convolvatron wrote: post #21
mpalczewski wrote: post #8

First thing I review is readability. Code should be readable. Once code is readable it makes reviewing the rest much easier. Readable code is more maintainable and problems jump out at you. Stuff other than readability is important, but if you focus on readability it makes the rest of the review go very smoothly.

unfortunately, 'readability' is fairly subjective. I was part of a group recently that spent a good couple hours every week arguing whether 'ctxt' or 'context' was more readable. when confronted, they explained to me as one would a child, that they cared deeply about code quality

[deleted]

Re: The Code Review Pyramid

#28

Post by Kranar »

willcipriano wrote: post #18

The more experienced I get the more I want to work on teams with a higher degree of trust than ones like this. Don't get me wrong I think code review can be valuable but on high preforming teams it works best on a as needed basis. The way this has worked successfully for me in the past is as a new joiner to a team you submit for code review every time. My first couple of reviews tend to have a good bit of feedback as…

I feel I work on a high performance team and code reviews are a joy. When I submit my code for review it's an opportunity to share my work with my peers, potentially explain my thought process and my work process and we get to learn from one another, and when I get code to review it's the opposite, I get to see how my coworkers think and tackle problems.

I have never once felt like a code review did anything to erode trust and I don't see how a code review could slow down a work process. Using git, as soon as I submit code for review I begin work on the next task and if I need anything from the branch I submitted for review, I just merge that into the new branch. If changes are requested I make the changes and then merge again.

Code reviews are among the best ways of ensuring that code remains consistent and that everyone is learning from everyone else. That's why in my team all pull requests are reviewed by two people including one junior developer.

Re: The Code Review Pyramid

#29

Post by AngeloR »

I've been thinking about code reviews a lot.. and I think a big problem with existing code reviews is that for most orgs, code reviews were a way to move planning to the end of the development process instead of keeping it up-front.

I think the foundation of the pyramid (API and Implementation semantics) is often discussed in code reviews when it should have almost no place. By the time you get to the code review not only should these have been ironed out and shared with the whole team. What you should be looking at is simply adherence to the plan, and calling out deviations.

I wrote about a small rat about this: https://xangelo.ca/posts/code-reviews-are-failure/

Re: The Code Review Pyramid

#30

Post by RussianCow »

tuyiown wrote: post #11
simonbarker87 wrote: post #7

Earlier quoted context omitted.

I’m at the point now where I don’t want to think about layout and formatting of my code, just let a formatter do it all for me on save please

But, code formatting is not a solved problem, right ? You either have to fiddle until the formater outputs proper layout, or let it rest as is and never learn how to format your code, letting the code base leaking tons of probable less than ideal layout.

I configure Prettier once on my JS codebases and then never touch it again. Seems about as "solved" as is reasonably possible.
Post reply on HN