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

91–100 of 117 posts

Re: The Code Review Pyramid

#91

Post by TheCoelacanth »

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

There's a simple test for readability. Someone familiar with the codebase reads the code and tries to understand what it does (a.k.a. a code review).

If it takes a low effort, then it's high readability. If it takes a high effort, then it's low readability.

Re: The Code Review Pyramid

#92

Post by TheCoelacanth »

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.

Readability has very little to do with styling.

It's about clearly and concisely expressing ideas.

Leave styling to linters. Reviewers should be focusing on how the code is communicating.

Re: The Code Review Pyramid

#93

Post by TheCoelacanth »

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…

In perfectly readable code, it's easy to notice performance problems.

In my experience, unreadable code is usually where the worst performance problems are, because that's where they are the hardest to find and fix.

You seem to be conflating readability with formatting, which is not what I mean by readability. Poor and inconsistent formatting adds some friction to reading, but it's a relatively minor part of readability.

Re: The Code Review Pyramid

#94

Post by AngeloR »

gunnarmorling wrote: post #54
AngeloR wrote: post #29

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…

> 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. That's a fair point. It's my background in open-source and I suppose the specific ways we work in the projects I'm involved with which made me add these things at the bottom. Oftentimes, contributors come up with PRs for new feature…

This is 100% a side-effect of GitHub. Open Source used to have the additional barrier of mailing lists to detract from "drive-by PRs". The side effect, of course, is that it made it much harder to get involved. GitHub really brought a lot of new eyes on open source software and lowered the barrier to contribute exacerbating (an argument could be made for creating) the drive-by PR problem.

Re: The Code Review Pyramid

#95

Post by watwut »

cillian64 wrote: post #65
boredtofears wrote: post #59

Earlier quoted context omitted.

I don't disagree with you at all, but I'm not sure I've been a part of an organization that does it right. Beyond the stuff like style, conventions, and obvious bugs/problems, it's challenging to provide meaningful insight to someone that has many more hours of experience in a particular area of a codebase more than you do. I know I personally hold back from calling out things that I find to be subjective -- even if…

You assume here that people only work on the area of the codebase which they are most familiar with. I think a sign of a healthy team is when people are contributing to other parts of the code and not getting stuck in a rut where each area of code only has one maintainer/gatekeeper. Also, for less experienced members, reviewing the changes of more experienced people is a great way to learn if you properly sit down an…

> In addition to catching mistakes, this is useful to check that code is actually comprehensible to people other than the author and a good time to request more/better comments if necessary.

The most convoluted code I ever worked with were in teams that treated code review as secret cow. It happened twice. The best was in teams without code review, but with strong ownership.

The issue is, I think, that code review is myoptic. It ensures surface level readability and does very little for actual structure. And I think it gave people false confidence and made them less willing to refactor.

Re: The Code Review Pyramid

#96

Post by [deleted] »

sanderjd wrote: post #85
boredtofears wrote: post #66

Earlier quoted context omitted.

I’m not assuming that at all. When you’re at a company that has $Xmm lines of code floating around in its repositories, it’s unrealistic to assume you will be able to cross functionally train all developers on all code. Of course you try to aim for that, but you’ll inevitably find yourself in situations where you don’t.

Not all developers on all code, but N developers on any given piece of code, where N should be greater than 1. Why were you asked to do the review? Is N equal to 1 so there is nobody besides the author who has the expertise needed to do the review? That's a good opportunity to start the process of raising N to 2. Is it because N is equal to 2, but the other person is out on vacation? Better start working on getting N…

[deleted]

Re: The Code Review Pyramid

#97

Post by smokey_circles »

One thing I wish I knew was coming was what my first code review as the tech lead was gonna be like. I was as unprepared as the grads code was.

Things I've learned about my own professional code preferences since

1) I will not sacrifice readability for any reason. That is the hill I'm willing to die on. Reducing code is sometimes hard and that's fine that people like to challenge themselves by doing so, but not at the expense of readability. You will forget the clever trick you employed, I promise

2) Business objectives are second class to system stability. This is because the primary business objective is to be online. Failing a few transaction is bad. Failing all the transactions is much worse.

3) There is no such thing as self documenting code. There is just good nomenclature. Documents always improve someone's experience. On boarding documents for new devs will save you so much trouble.

4) I'm a huge fan of in memory caches. I hate seeing them in code. When they work, they're wonderful. But when there are issues, they're closer to demons than to bugs.

5) Data Structures. Data Structures, Data Structures, Data Structures. And then more Data Structures. Easy pre-optimisation, and these days it usually just means knowing when not to use "the default" data structure (such as ArrayList in java)

7) One day (if you're not there already), you will be on the otherside of this review. You're almost guaranteed to remember the bad times, so pay extra special attention to when the process is working well.

A simple trick I've learned is to repeat a process that worked from a previous role at a new role who's process is not working. Works more times than not.

Re: The Code Review Pyramid

#98

Post by smokey_circles »

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 see 2 clear problems with this ideology:

1) There is too much knowledge to fit inside one person's head. Cooperation is the keystone of success.

2) Your team will grow. You will need to share this knowledge somehow before the trust can be given

But in defense of this view point: code reviews shouldn't be the opposite of what you're talking about.

I trust my teammates to catch any mistakes I've made. I'm human and therefore fallible. It's going to happen.

I trust mistakes they've made are for the same reasons.

What you're trusting is your teammates are top tier 1% every time and bordering on being super human.

I'm trusting that my team has my back and we're in it together.

Re: The Code Review Pyramid

#99

Post by eorn3452gl »

smokey_circles wrote: post #97

One thing I wish I knew was coming was what my first code review as the tech lead was gonna be like. I was as unprepared as the grads code was. Things I've learned about my own professional code preferences since 1) I will not sacrifice readability for any reason. That is the hill I'm willing to die on. Reducing code is sometimes hard and that's fine that people like to challenge themselves by doing so, but not at th…

> "knowing when not to use "the default" data structure (such as ArrayList in java)"

Can you give some examples of when not to use ArrayList?

Re: The Code Review Pyramid

#100

Post by dhzhzjsbevs »

> When it comes to code reviews, it’s a common phenomenon that there is much focus and long-winded discussions around mundane aspects like code formatting and style, whereas important aspects (does the code change do what it is supposed to do, is it performant, is it backwards-compatible for existing clients, and many others) tend to get less attention.

Code formatting and style discussions that don't start and end with, hey, our code formatter is broken have absolutely no place in code review. Also, keep your damn nits to yourself. If it's a nit, it's not important.

Post reply on HN