Hacker News new | ask | show | jobs
by zhemao 3694 days ago
I think a benefit not mentioned in the article is that it makes sure that at least one person besides the original author understands what the code does.
4 comments

It's very important, and also it makes sure everyone is aligned about how to evolve the codebase, the project is in some coherent state and you can reason about the code as a whole (same patterns over the whole codebase etc.).

Also with code reviews you make sure other devs are not (re)introducing some bugs that have been fixed before (maybe in other places of the app), and so on. There can be objective technical bugs, but code review will catch also the misconceptions about how stuff is supposed to work in the app, which can have profound consequences - no matter how great your code is technically, it you're building a square while circle was requested, it's not good.

I'm absolutely stunned by the comments in this thread. I've worked for a short while in a team with no code reviews, everyone was pushing whatever the hell they wanted and it was a nightmare (and git history was a total spaghetti). Soon after I joined the pull request workflow was introduced but it was too late, the code was so bad it was unmanagable.

Unless you have really magnificent team, chance are too high that someone will be writing non-maintainable/non-debuggable code, using super confusing variables/methods/classes names, or reinventing the wheel, or not handling errors properly. "We don't do code reviews" is a no-go for me at this point, second to only "We use ClearCase for version control".

Unfortunately code reviews don't ensure this. Reviewers, when faced with code in a domain they don't understand, don't go out of their way to learn it; instead they just look for surface-level / nitpicky stuff.
That's not a problem with code reviews; it's a problem with your company's implementation. "Please explain this" should be considered a very reasonable response to a code change the reviewer doesn't understand. In my experience, this ends in the code being cleaned up with readability in mind and the reviewer learning something new in more or less equal proportions.
It all depends on the reviewers, some will spend expensive hours on finding minor code style issues and not understand what actually happened in the commit.
absolutely agree -- situational awareness of changes in the codebase, this is the number one priority. Even if a reviewer does not understand much of it, when shit happens after merge, they know why. A new issue might be related "to that thing I read in so-and-so's review the other day"