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

41–50 of 117 posts

Re: The Code Review Pyramid

#41

Post by larusso »

bmj wrote: post #34
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…

On my team, the level of scrutiny a merge request receives depends on the author and the functionality. MRs from a principal or lead engineer tend to get rubber-stamped at a higher rate, though, to your point, many of us will explicitly call out code that we would like to have reviewed in more detail. If a more junior level developer submits a merge request, it is highly likely their code will get a close review. I'm…

I‘m called a principal engineer in my organization (I know that can mean everything or nothing) I like to use code review requests and sometimes gates also to put a fence in front of my ego. Also this is a great way of sharing knowledge. I had great sessions where questions about X and then answers verbally or in text suddenly put stuff into different perspective. Sometimes I realize that my solution might be to complicated or an more generic, performan, (paste fancy adjective to describe your code here) is easy to achieve.

Re: The Code Review Pyramid

#42

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

#43

Post by b3morales »

Quarrelsome wrote: post #33
pc86 wrote: post #16

Earlier quoted context omitted.

Potentially unpopular opinion, but I would much rather get well-formatted, performant, readable code that misses the business case a bit than get a ball of spaghetti mess that nails it. Not only will it be easier to update the code in the former, but the business pressure to just deploy the latter and "fix it later" can sometimes be too high to resist. I was just looking at a "proof of concept" with a 2017 commit dat…

No, I mean like the primary function of every PR is to ensure the code does what its trying to do and doesn't (for example) fail to compile, or break the build or create an infinite loop or an out of memory exception.

On a dayjob closed-team project, if my coworkers are putting up PRs without making sure they meet functional requirements, that is a problem that needs to be solved several steps before the PR. If it's a junior person who doesn't know better, more working side-by-side with them to get to the PR. If it's a senior person dealing with a shitty spec, more discussion with PMs/designers before we start coding. If it's a senior person who's putting up broken code because they don't care...that's probably a management conversation.

I have to be able to trust that my teammates are doing their best to produce working components. Otherwise code review is a bandaid covering a hole in the hull of a sinking ship.

Re: The Code Review Pyramid

#44

Post by kolanos »

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'll take a consistent less than ideal layout over whatever $HUMAN comes up with. But to me an inconsistent layout is the less than ideal one. And humans excel at inconsistency.

Re: The Code Review Pyramid

#45

Post by chimprich »

nine_zeros wrote: post #19
sgeisenh wrote: post #14

Earlier quoted context omitted.

I think you're missing the point: it's incredibly hard to determine functional correctness of unreadable code. So the first priority is getting the change to a readable state. I'm not talking about nits, I'm talking about minimizing the cognitive overhead to truly understand what the code is doing. That being said, it is also easy to identify nits during this phase. In codebases that have collections of best practice…

> I think you're missing the point: it's incredibly hard to determine functional correctness of unreadable code. There is a spectrum between style differences and absolutely unreadable code. If it is absolutely unreadable, sure, send it back to be readable. But if it is merely styling issues, I'd encourage you to understand that you are not saving any time by focusing on styling before functionality.

> I'd encourage you to understand that you are not saving any time by focusing on styling before functionality.

Far more developer time is spent reading code than writing it.

If you can speed up the time required to understand a piece a code by improving the style then it's almost always worth it. For a professional software engineer, just above "absolutely unreadable" is far too low a bar to aim for.

Re: The Code Review Pyramid

#46

Post by quadrifoliate »

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…

The popularity this sentiment on HN terrifies me.

Please don't do this. I keep finding rubber stamp code reviews (or none at all) while diving into codebases for where bugs were introduced, and 9 times out of 10, it's someone who didn't bother to get proper reviews for their code because "it's just something slowing me down". I can assure you that you have bugs and usability issues in your code, not just "linter issues". Everyone does.

If your code reviews are becoming rubber stamps, that's an indication that you need to slow down and teach other team members about the system, or hire smarter people who can understand your reviews more quickly. Don't respond by getting rid of the review process as a knee-jerk response to these sorts of organizational constraints.

Re: The Code Review Pyramid

#47

Post by kolanos »

ilovecaching wrote: post #25
kolanos wrote: post #5

Earlier quoted context omitted.

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.

As long as the starting point is what an opinionated formatter spits out, then we can talk about the hardest thing in software engineering (naming things). But lets be real, most "code style" conversations are of the formatting variety and they're the purest form of bike shedding.

Re: The Code Review Pyramid

#48

Post by watwut »

pc86 wrote: post #12
nine_zeros wrote: post #9

Earlier quoted context omitted.

If the code is doing the wrong thing in the first place, no amount of nitpicking on readability will save time. First pass: Functional correctness Second pass: Better ways to do the same thing (optional) Third pass: your favorite nitpicks.

This structure (especially the optionality of the second pass) is how you end up with perfectly readable code iterating over List when a hash map would suffice and be orders of magnitude more performant. Nobody can do it perfectly, but I think trying to keep personal nitpicks out of reviews as much as possible - ideally by codifying team nitpicks in automated formatting/.prettierrc/whatever - lets people focus on thi…

Then again, that one is super easy to find and fix if it turns out to actually be problem.

Re: The Code Review Pyramid

#49

Post by sublimefire »

sz4kerto wrote: post #2

Ours is different. Tests (end-to-end and integration) are at the bottom. If there are no tests that prove that the code works, we won't really look further because we don't even know if the code implements the right thing. Then comes interfaces, then implementation, then style.

What is more the tests will highlight the API and the expectations (see API semantics in the pyramid). By nature, the process of writing tests will usually catch some bugs. I'm not arguing for the tests to cover each possible scenario, but they must exist. If, on the other hand, it is hard to add the tests then it probably means you have bigger problems.

Re: The Code Review Pyramid

#50

Post by mannykannot »

pcmoney wrote: post #32
mannykannot wrote: post #6

While I recognize the problem as real and significant, and I think a hierarchy of concerns is a valid way to mitigate it, I feel there are a few problems in this particular pyramid. Of all the issues that might be raised in a review, which are the most important to fix? I find it difficult to imagine a ranking in which incorrect functioning (including security vulnerabilities and truly inadequate performance) are not…

If your team does not share a subjective understanding of what makes their product/API useful none of this matters. Focusing on objective technicalities is the least useful aspect of code review. If you have someone who constantly is like "Well that's subjective" and derails conversation that way you need to coach them or move them off the team.

Are you saying that if you enter a code review without having a shared subjective understanding of what makes your product/API useful then the review is not likely to be successful, or that code review is the place to form such a shared subjective understanding? I can agree to the first interpretation, but about the latter I would say that code review is quite a bit later than optimal, and that there are other, better ways of achieving it.

If your organization is such that you cannot even discuss these issues until you have working code, then you have other, bigger, problems.

Post reply on HN