Live data from Hacker News

Code review can be better

tigerbeetle.com

121–130 of 253 posts

Re: Code review can be better

#122
post #81

What bothered me for a long time with code reviews is that almost all useful things they catch (i.e. not nit-picking about subjective minor things that doesn't really matter) are much too late in the process. Not rarely the only (if any) useful outcome of a review is that everything has to be done from scratch in a different ways (completely new design) or that it is abandoned since it turns out it should never have…

Where I work we tend to write RFCs for fundamental design decisions. Deciding what counts as a "fundamental design decision" is sometimes self-moderated in the moment but we also account for it when making long term plans. For example when initially creating epics in Jira we might find it hard to flesh out as we don't really know how we're going to approach it, so we just start it off with a task to write an RFC. The…

I view this process like this: code review is a communication tool: you can discuss concrete decisions vs hand waving and explaining in the conceptual space, which of course has its place, but is limited.

But writing the whole working code just to discuss some APIs is too much and will require extra work to change if problems are surfaced on review.

So a design document is something in the middle: it should draw a line where the picture of the planned change is as clear as possible and can be communicated with shareholders.

Other possible middle grounds include PRs that don’t pass all tests or that don’t even build at all. You just have to choose the most appropriate sequence of communication tools to come to agreements in the team and come to a point where the team is on the same page on all the decisions and how the final picture looks.

Re: Code review can be better

#123
I have been working on the PR implementation for lubeno[1] and have been thinking a lot about the code review process.

A big issue is that every team has a slightly different workflow, with different rules and requirements. The way GitHub is structured is a result of how the GitHub team works. They built the best tool for themselves with their "just keep appending commits to a PR" workflow.

Either you need to have enough flexibility so that the tool can be adapted to everyone's existing workflow. Or you need to be opinionated about your workflow (GitHub) and force everyone to match it in some way. And in most cases this works very well, because people just want you to tell them the best way of doing things and not spend time figuring out what the best workflow would look like.

[1]: https://lubeno.dev

Re: Code review can be better

#124
post #81

What bothered me for a long time with code reviews is that almost all useful things they catch (i.e. not nit-picking about subjective minor things that doesn't really matter) are much too late in the process. Not rarely the only (if any) useful outcome of a review is that everything has to be done from scratch in a different ways (completely new design) or that it is abandoned since it turns out it should never have…

> It always seems as if the code review is the only time when all stakeholders really gets involved and starts thinking about a change.

That is a problem with your organization, not with Git or any version control system. PRs are orthogonal to it.

If you drop by a PR without being aware of the ticket that made the PR happen and the whole discussion and decision process that led to the creation of said tickets, you are out of the loop.

Your complain is like a book publisher complaining that the printing process is flawed because seeing the printed book coming out of the production line is the only time when all stakeholders really get involved. Only if you work in a dysfunctional company.

Re: Code review can be better

#125
post #14

> When I review code, I like to pull the source branch locally. Then I soft-reset the code to mere base, so that the code looks as if it was written by me. This is eerily similar to how I review large changes that do not have a clear set of commits. The real problem is working with people that don’t realize that if you don’t break work down into small self contained units, everybody else is going to have to do it ind…

I use this: https://github.com/sindrets/diffview.nvim

as a PR review tool in neovim. It's basically vscode's diff tool UI-wise but integrates with vim's inbuilt diff mode.

Also, `git log -p --function-context` is very useful for less involved reviews.

Re: Code review can be better

#126
post #102

Earlier quoted context omitted.

It still is engineering you only mistake design phase. Writing code is the design phase. You don’t need design phase for doing design. Will drop link to relevant video later.

I also read this series of blog posts recently where the author, Hillel Wayne, talked to several "traditional" engineers that had made the switch to software. He came to a similar conclusion and while I was previously on the fence of how much of what software developers do could be considered engineering, it convinced me that software engineer is a valid title and that what we do is engineering. First post here: http…

I was an undergraduate (computer) engineer student, but like many of my friends at that time (dot-com boom) I did not graduate since it was too tempting to get a job and get well paid instead.

However many, probably half, that I work with, and most that I worked with overall for the last 25+ years (since after I dropped out) have an engineering degree. Especially the younger ones, since this century it has been more focus on getting a degree and fewer seems to drop out early to get a job like many of us did in my days.

So when American employers insist on giving me titles like "software engineer" I cringe. It's embarrassing really, since I am surrounded by so many that have a real engineering degree, and I don't. It's like if I dropped out of medical school and then people started calling me "doctor" even if I wasn't one, legally. It would be amazing if we could find a better word so that non-engineers like me are not confused with the legally real engineers.

Re: Code review can be better

#127
post #102

Earlier quoted context omitted.

It still is engineering you only mistake design phase. Writing code is the design phase. You don’t need design phase for doing design. Will drop link to relevant video later.

I also read this series of blog posts recently where the author, Hillel Wayne, talked to several "traditional" engineers that had made the switch to software. He came to a similar conclusion and while I was previously on the fence of how much of what software developers do could be considered engineering, it convinced me that software engineer is a valid title and that what we do is engineering. First post here: http…

Personally I don't need to talk with "traditional" engineers to have an opinion there, as I am mechanical engineer that currently deals mostly with software, but still in the context of "traditional" engineering (models and simulation, controls design).

Definitely making software can be engineering, most of the time it is not, not because of the nature of software, but the characteristics of the industry and culture that surrounds it, and argument in this article is not convincing (15 not very random engineers is not that much to support the argument from "family resemblance").

Re: Code review can be better

#128

Earlier quoted context omitted.

That’s basically an async pair programming session, isn’t it?

Yes I think so. Have you tried it?

Just cases where PR were submitted close to someone holidays and I assigned it to someone else in the team to bring it over the line. But otherwise I have worked with sync pair programming only.

Re: Code review can be better

#129

Here's an alternative I've wondered about: Instead of one person writing code, and another reviewing it - instead you have one person write the first pass and then have another person adjust it and merge it in. And vice-versa; the roles rotate. Anyone tried something like this? How did it go?

Great idea, if you're fine with development time to take twice as long.

Re: Code review can be better

#130
post #120

Earlier quoted context omitted.

Stacked pull requests seem to add a layer of complexity to solve a problem that should and can be avoided in the first place. Frequent, small changes are really a good practice. Then we have things like trunk-based development and continuous integration.

Stacked PRs allow me to post frequent, small changes without waiting for a review between each one.

Well, you don't need stacked PRs for that...

I think stacked PRs are a symptoms of the issues the underlying workflow (feature branches with blocking reviews) has.

Post reply on HN