The author wouldn't say this if you'd seen some of the code that I've seen juniors make a PR for.

Code reviews are essential when dealing with juniors or newcomers into a code base..

The company I worked for liked to hire people who couldn't actually code (or speak English) just so they could have a "diverse" workplace, so I was constantly tasked with stopping their service-breaking code from making it to production. If I wasn't reviewing their commits, the company would not currently exist.

Coworker: This function seems too complicated for me. Me: Skill issue. *merges anyway*

> For instance, sharing about coding guidelines, architecture, coding practices At my job we have a code style that the whole department should follow, we ask developers to install IDE plugin that will format code automatically, we ask developers to install git-hook that will format code before commit, we added a build-plugin that will fail compilation if the code doesn't follow the code style. And people still didn't follow it. Many people over-complicate stuff, they write code that doesn't make sense, they can make a simple for loop make N^2 number of iterations. Even "senior" developers that apparently have 7-11 years of experience do all of this. And I'm not even talking about devs who implement 3-5 features in a single PR and then expect others to review all of this in a couple days

I worked at a French startup called Sunday that did trunk based development and pair or mob programming all the time. It was painful but boy did we fly. The speed at which we added changes was nothing like I had ever seen. The difficulty though was finding the right person to pair with and in my case the timezone difference but it was extremely effective. Stressful, but fast.

Advocating pair programming instead as an absolute is silly. Some problems are NP-complete. They might involve hours, or even days of programmer time, but adding a second programmer at best reduces the wall-time by minutes (and at worst increases it by hours). Once the solution is written, verifying it works is _much_ easier than learning all the dead-ends that were explored and discarded along the way. This most often comes up when trig or calculus shows up in a non-trivial way. Having someone along for the ride while I'm working on a whiteboard figuring out which trig identities to apply to find some value either has the second programmer sitting idle, or asking questions and grinding progress to a halt. But once the code is written (complete with comments explaining which identities are applied), that second programmer can verify the chosen identities are correctly applied quite quickly (and can verify they _are_ the correct identities if required).

We tend to forget that web development with a 2-year code lifecycle till obsoletion is not all software. Code reviews look differently and have different purpose if you develop software for heavy industries, cars, aircrafts - you design the code for a decade ahead. Sustaining healthy architecture and avoiding hack-arounds is a very important objective for those platforms and it is often painful for developers to conform and develop tedious and substandard solutions in the name of preserving the idea that the architect had.

If you're doing pair programming you still need someone else to do the code review.

Often the real problem is a social one. If you work with people who hate their job/project then they either: procastinate with excessive code reviews in order to not have to do their job or they see it as part of their job and do nothing/ghost or tldr. Making code reviews a bad thing. On the other hand if people like their job/are passionate, then you get great feedback and code reviews are good.

Problem is not Code Reviews, problem is when developers create a mega pull request with 20 files changed. Instead of changing the way the code is reviewed, we should change the way MRs are created. Splitting a Mega MR into smaller ones is always a great way and almost always possible.

As a very junior developer. I started writing my first PRs a few months ago. And I really enjoy the idea of having the affirmations of a code review. I got to excited and opened a nit PR on other repos (like adding hyperlinks that were missing in the readme). And got shot down. Which demotivated me there. Now I am asked to maintain the feature myself as its own repository and still having a reviewer approve it is great. I also like how clean the repository is if all commits a PR merges.

That article is crazy talk to me. Code Reviews on Pull Requests is the most important thing imho. Of course, for larger changes, talk about it first - but still, a PR is the place to talk about issues that come up with the code that is coming in, but also with the code that is already there. At this point, you have Code. Code it fact. No fancy architecture diagrams to hide behind.

I feel like most of these could be solved by simply establishing a few guidelines for reviewers, like spending X hours a day on reviews, assigning reviewers, allowing reviewers to remove themself if they are too busy, letting merges through and creating a separate tech debt to address more complicated issues, etc. At least, I don't really have most of these problems. Honestly sounds like a junior dev who got dinged too hard a few too many times for his code.

I've done code reviews on a project which doesn't have linters and formatters set up, where a more senior developer still just let his editor go crazy with the auto formatting... It was pure hell to review... I do however honestly think that most bad things about code reviews happens because of bad practices. E.g. doing many things in the same PR, leaving PR's hanging, and so on.

if you aren't having an explicit set of meetings to decide: 1. enforcing code styles via linters & formatters 2. code review comment thread ping pong limit (1 allotted back and forth then get on a call and resolve the thread with a final comment) 3. r.o.e. for tagged reviewers (dm reviewers directly when pull request is created and then again after 24hr of inactivity) 4. removing subjective nonsense from reviews (nit picks). track and put these topics in the next meeting and add to step 1 5. naming conventions (code/branch) then what is your team even doing?

On top of the whole Junior angle, CRs are also a good chance to be informed as to what other teams are doing and new approaches to problems that you might encounter elsewhere.

After having worked with trunk based commits and peer programming, I have to say that code reviews should be done in-person/via voip. TALK TO EACH OTHER. Literally discuss the code while looking at the code. Communication is SO UNDERRATED in the programming world.

As you kind of touch on, code review policies and etiquette against open source / random contributors is different than inside a proper dedicated project team familiar with the project and product goals. They should be two separate discussions, imo. One size does not fit all.

No matter what experience level you're at code reviews are absolutely essential. They help spread information about what's going into the code base and they're a useful learning tool. If it's a bottleneck, then do them post commit, but you should still do them.

Pair programming is useful in certain circumstances. Good code reviews are useful in all the cases where pair programming is impractical or otherwise not workable. Yes, you do need to mentor people on CR expectations to avoid many of the traps. If your process allows pair programming to count as an automatic code review, you should find some fast groups naturally appear in your teams.

I once had to resolve conflicts on my pull request for 10+ times and I get blamed for too long. Wish we could get a demo at companies before actually joining them. I would leave then and there if I could.

As senior software engineer on a project of mine... the other coders have the ability to merge their requests into DEV and test it themselves. I don't normally see what they put in DEV until after they say it's good... if their changes look normal to me I'll build & deploy it to Staging (via CI/CD). If I see a change that confuses me I'll slack them and ask about it. Once in staging QA and some people that requested the change will test it and let me know if good. If everything is good I push to Production. When I push my own code I still make sure QA and people test it. I'm not perfect. Then goes up the CI/CD pipeline to production. I really spend little time reading others code unless something sticks out to me.

We do mostly mob programming at where I work (100 devs, single product). Motivation is that each team (3-4 devs) owns and is responsible for one or few services and everyone needs good understanding of and agreement on changes. PRs don’t provide that level. So we avoid these kind of issues all together. All discussions happen synchronously and early on. It can be challenging when teammates have different rhythms. I have ADHD and need a faster pace while others find it a bit exhausting. But when you find a good match then it’s incredibly efficient and enjoyable.

Prime is talking about what I believe is the way humans really learn. (And AI BTW) You take your best guess and see how that works out. So you try your best to make the code changes and then you submit them, is this good? And since you're not familiar with the vision for the project, you get corrected and learn. You repeat and try again. For those trying to learn to code, Just read some tutorials, doesn't matter the language but it should be good at what you want/need to do. And try and build something. It ain't going to work but you then try to figure out what's wrong. Maybe, what you're trying to do is harder than you realize, that's okay you're learning about what it takes to build a real program, simplify and try something else. Then come back to it.

We moved from code reviews to mob/pair and it was excellent. Much smaller changes could be made and much faster. People tended to batch changes because of th over head of the PR process.

The "redesign" is not normal. It happens when devs rush to the keyboard instead of reviewing specs and design earlier in the development proces. It's a definite smell in my book. The primeagen: "he just didnt know what i wanted", That shit works when you own the project. In a 9-5 work team, this attitude gives you an insta -100 points.

I think it's more important to improve the code review experience than getting rid of it. The alternative is that someone made a change without discussing with others and it could result in merge conflicts, bugs, or unintended behavior. Maybe you could argue that if you're working on a project mostly alone, then code reviews are excessive, but when you do collaborative coding, you'll need some sort of stability in the code base.

It helps to know if the code matches the ticket.

Code reviews are only good if the reviewer knows what they are doing

this was super triggering. I have trauma from working with people who do not know how to do code review effectively to save their lives. The reality is the people who know the least about software are the most picky and hard to please and often require the code to be significantly worse before they will accept it because they have memorized 6 dogmatic rules of clean software or some other scam.

I feel like a lot of the problems the author has would be solved with small teams and requesting a review from one other dev for each MR. Edit: scratch that, it would all be solved by them actually talking to their damn colleagues.

I literally have a PR for the Dapr rust SDK that has been open for six months at this point waiting for a review. They was quick to review the first iteration and wanted some minor changes. Changed them and then dead silence after that lol.

When I was a QAM, I sat down with the developer and discussed, knowing what was wrong and what was a better way to do it. That way, the developer knew why it was incorrect and why the new implementation was better. I had a developer who told me, "fuck you," because his code was full on SQL injection and XSS. 🤣 To top it off, the management was not backing me up (I was free-floating). Then, they got hacked by a gray hat, and they understood how important it was to fix those. I fixed many of those myself, then took the time to explain how prepared statements work and how to sanitize for output in HTML. One guy left, and I was a "plus" for another guy. He wanted to go because there were not enough challenges, and I could retain him until I quit because I told him that in other companies, he may not have someone to show him cool stuff. In fact, when I left, the team dissolved in a few weeks. 🤣 Basically, if you're a QAM, you should do your job with passion and grow others.

If I do a review and notice some more style/opinion issues, then I approve it but still out those comments. So it's not required to be addressed, but just expressing that I think there's a cleaner way to do it. I do the request changes when the way it's done is more significantly wasteful or misses something important about ensuring it works consistently.

The articles issue with the "redesign review" was pretty bad... It's demoralizing? What the heck does that matter? If the way they went about solving the thing or making the feature has major design issues, either from lack of knowledge of the stuff, over engineering, or whatever.

I’m camp ⛺️ “separate refactor PR”

8:26 preferring changes with refactors sounds absolutely crazy to me.

My take is that there should be a predefined style (i.e., a linter or code formatter...style reviews are useless!). Constructive code review is subjective, but it does help catching small error typos or maybe things that shouldn't go into production! At least for projects beyond a certain size. Yes, some code reviewers are nit picky, slow and most of the time self-serving! But that is the price to pay for quality!

13:40 Just six different projects?! Oh man, that would be nice. At my last gig I had to be the master of nearly 22 different projects. BTW, I'm looking for work. I'm excellent at Node.js (TS/JS), dotnet (F#/C#), and python. I've also learned Rust and have written real projects in Rust. I'm excellent with Docker, Postgres, and the myriad of GNU tools!

What we generally do at our company is quite efficient i suppose. We deploy once at the end of the sprint (Sometimes longer) and we deploy in Versions. So each time we make a PR there is a version attached to it, if it’s that of the current version that needs to be deployed, the product manager makes sure that these PR are reviewed by reminding the team about the priorities. And this kind of works although there is a bit of micromanagement when you give a team a list of prios for a sprint things generally work out in favor of that.

I agree too that it's the responsibility of the person doing the work to shuffle it through any team processes. Need a PR from a certain person? Ask them. Need it fast? Ask them. They are busy? Find someone else. None of this assign and forget. Own your work.

I don't always understand Prime since I'm not a Dev haha. But I can relate to that disdain in the "Shut up" 1:29

My team enforce a design-first approach, and it makes me frustrated. I just wanna get in there, make some changes, and figure out what's going on. There's a huge team of non-coding architects of course

Assuming we're talking about a team of developers on roughly the same skill level, to avoid problem code or wasted effort, the most valuable use of time is to simply discuss the approach for implementing a feature. The more obvious the feature, the easier it is to agree, the less time is spent, the faster you get to start coding. More complex things will result in team members needing to answer open questions by pair/mod programming. For truly complex features/epics spanning months, it helps to plan things out in advance with a design document. You don't need to figure out every minute detail, but you need to make sure the envisioned structure holds water and doesn't explode. That's just a fact of engineering complex systems.

7:59 - No actually the rebase thing *really is* a problem. The rebasing itself isn’t the problem, though… it’s like a combo of the ghost reviewer w/ rebasing. You issue a PR that’s up to date (properly based), wait forever, they come back ask you to rebase, you do it (maybe multiple times) and the PR goes nowhere. 😫

idk about this article, code reviews to me are about teamwork and if you see that as being counterproductive, that means you aren't willing to compromise. imo if you aren't using IRC, getting in a call, or talking in person, nebulous PRs are going to be a nightmare for both parties.

Basically all the points in the article are about (mis)communication. Code reviews are a pretty great way of communicating code, I don't really see how communicating earlier solves any issues without introducing new ones. The clear offenders of annoyance in the list can be solved with non-blocking reviews: Nit as much as you like but clearly communicate what piece of code and why is blocking an approval. Variable names, formatting and structures aren't 'blockable offenses', it should be left to the PR holder to incorporate suggested changes or not.

I wish we could just merge things that are waiting for review, or only 2 people can approve for a particular branch and they are both out on pto. but regulated entities prevent this :(

It's always fun when people talk about the architecture and then you go read the code and it doesn't follow what they say it is. Then you find out they're also new to the codebase and that's their best guess at the architectures and problems with the system. I strongly believe that code is the source of truth. If you want to understand the architecture go read it. Additionally, if the coding style is so abstracted with declarative behaviour where there's some metaprogramming library attached to class definitions (looking at you java). You should seriously consider whether you actually get a benefit from hiding all those features from the person trying to read into the behaviour of a system.

The author is so incapable of communicating with their colleagues that they wrote this article about the things they do/don't want their colleagues doing instead of communicating that with their colleagues.

OMG I had such a code review today, in a project where the conventions were completely verbally passed along to the devs every time a new dev gave an MR So I had to resolve a lot of comments, all of which were along the lines of "use length instead of checking isEmpty" or "use arrow functions instead of regular functions" And when asked for their reasoning, its always "its the (unwritten) convention that we follow for this particular project, no real reason" Like, wtf

I literally cant comprehend why someone would complain about his code being reviewed. I work in a company where every developer works on projects solo , imagining someone giving feedbacks and being able to improve you is ~chef kiss~

Typical webdesigner mindset.

Imagine working at the place that adheres to everything this article complains about. It’ll be a nightmare level mess in months. Just let everyone cowboy it all their work. Ok, good luck.

Chaotic projects make for chaotic code review workflows. Have yourself a mature team that operates well together and give them sovereignty of the repo. Congratulations, you now have time to do other things.

I wanna do this kinda stuff for a living... what do?

Code Reviews are mandatory if: 1. its a team effort 2. you want maintainable code base with normal learning curve for long period of time 3. all costs are covered by somebody outside the dev team (company, stakeholder…) In all other cases, do whatever you want….have fun…but usually its equal to jumping from airplane with no parachute ✌️😁

The author is talking about a situation where communication happens primarily through pull requests. If you're senior working with a bunch of juniors in this situation, you're going to have a rough time. You're going to have to reverse engineer the intent of the junior code and give constructive feedback on viable alternative approaches. Basically you're getting a bunch of dumb ideas thrown at you and have to analyze and provide correct solutions for all of them. The juniors are going to love it though, since they can just hack something together and get useful feedback on what they did. So you're getting voted down. From a senior perspective, that sucks and needs to get fixed. You'd rather have the juniors shoot themselves in the foot and learn the hard way rather than to get pressured to fix every juniors idiotic code. You end up doing the work of 10 people concurrently to "oblige with the process". I've been in situations like this and know that the struggle is real. The authors solution would work in this situation, but as many people point out in the comments, there are many ways out. "Just do your code review on time" actually feeds into the negative spiral though.

If we stop doing code reviews where I work, I can garantee you that the code base will be a burning tire field withing two months. Not because we code like first year programming students but because one or two juniors still need to get their shit together when it comes down to best practices.

Just came out of a job where working 9-5 in pair/mob programming was required. That shit mentally broke me. you have to be given the time and space to make mistakes and fail, that's where the real learning happens. Watching someone else cook wont ever make you a good chef, you need to burn a few cakes yourself

What is "java brain " means ?

Struggling is without a doubt crucial to develop problem solving skills but struggling for too long in a professional environment (out of the egoic desire to solve the problem entirely by yourself) can be counterproductive and eventually play against the team productivity especially when someone has the know-how. Therefore we should wisely balance the time to grind and learn and the time to be pragmatically efficient.

Change without refactor is better for easy code reviews. It's harder to review functional changes if every line of the file is in the code review diff. Change with a refactor is easier in the situation that you have to justify the business case for every change and create a whole JIRA ticket for the refactor, and story point it and all that nonsense.Teams with heavy ticket processes are incentivized to refactor + code change in the same ticket. It's a problem with the process and I think without that, they would probably want code changes and refactors in separate code reviews.

Does it count as a code review if I have chatGPT review the code it just wrote in another instance? (Asking for a friend)

...thank Baptiste Barbieri, Antoine Guy, Maxence Zajdlic and Rafael S. Ward... Let me guess, for reviewing the article.

Wish it was that easy, they can't take rejection quite well when it comes to PRs.

Code review guidelines should depend on the team working on the project. 5 junior devs and 1 senior dev? Absolutely, everyone gains from the experience. One single developer, and a "tech lead" blocking the merge on every nitpick? Maybe not the best idea. (Yeah you guessesd it, I'm currently on the 2nd case). Either way, I'm a fan of the "review time limit" idea. No comments for 2 days? Then I can merge without approval.

Near the end with your criticism of pair programming I realized something. I could never put my finger on why I dislike all the KZfaq “clone” tutorials where the person builds a Slack clone (e.g) and you just watch them and follow along. What you said about pair programming applies exactly to this. You don’t get the benefit of understanding all the pitfalls and the experience it took to get to that point.

So what I fathom from this article is that the solution to issues we face is in a distributed version control system is to not work in a distributed system, but just work together and it'll be good, and that kinda defeats the purpose. Just GIT GOOD! I do agree with type of reviews explained and how some of them can be an issue but not with the conclusion!

he got chat-gipidied

I think the itlte of the artice should rather be "Why you should do pair programming" but the author probably thought that then it would not get as many views.

Code readability can be maintained through review, as far as some agreed upon definition can be arrived at. Certainly consistency. And to consistency, certainly some describable testing posture can be arrived at and maintained through review. I've worked at a few companies with long lived projects and this may be a shocker for some, but you often have someone who is the lead engineer on a piece of software and they function as the arbiter of subjective truth. This lead and the top most senior engineers form a shared consensus on what's acceptable and not, and this flows through the project via code reviews and other discussions. Nits are the worst and the best teams avoid these or explicitly offer them as suggestions/ideas. You might think "there is no room for suggestions!", but maybe get over yourself just a bit! A second point of view, or tips working in a language you aren't super familiar with, can be very beneficial. Also, PRs with no description of what the problem was or how it was fixed is the worst for reviewers. If you spent 2 hours building up the context necessary to make the change, please share that context! Don't make your reviewer spend 2 hours re-figuring out what you did just to validate the changes :|

If the reviewer just goes through the whole PR first. Nothing worse then fixing a thing to have them point out some other things they didn't mention the first time around.

four projects in a year and a half? lol i am in over a dozen in under a year :( at least they are mostly java.. is that a good thing? Some python, js, and lots of yaml as well.. and no one wants to help, i am the only one thats doing any code reviews. but thats the nature of working for a cable company..

Code reviews become unproductive activities when a reviewer start ranting about his own preference on how code should be written. IMO code reviews are all about improving maintainability and reliability of codebases, for these someone's preference has little to do. The most important thing for code maintainability and reliability is, tests which allow developers to easily change or refactor codes without breaking something. When I review PRs submitted by seniors, I mainly just double check test codes to make sure if coverages are nice and they align with the specifications.

He had the conclusion in mind before writing the article. And so he wrote a great article with a shitty conclusion

How to get Knight Capped

We needed a code reviewer to block merging Imperial System.

Use the code review pyramid

I beg to differ when it comes to nitpicking. If you ignore the coding guidelines it's on you. It's no more work for anyone to follow those. As a c++ dev I usually work on huge codebases that are 15+ years old. There are enough styles in there already, just stay consistent.

I'm not sure why you have the different attitude for open source vs inside a company, for being happy to wait for reviews. I think your attitude for open source is the correct one for both cases. People can be busy inside a company too.

"just pair program everything" doesn't scale at all, and for me personally I need to think alone in quiet to be productive. Most of the time I'm requesting review from multiple people as well

So, basically we should stop doing code reviews because this one guy saw some bad reviews? Just go and have a conversation with these people who are leaving cryptic and useless comments on PRs..

What a trash article. Luckily Prime is able to bounce on it to make things interesting.

LGMT: Looks Good, Me Too

If your coworkers don't review your PRs in good faith and with a sense of urgency you have a teamwork issue. Those are not inherent in code reviews. Code reviews just expose those issues. And I find it weird how this article forces every point into a "The X review" format. Doesn't make sense.

Agree with Prime

LGTM

If there is re-design issue during code review, you have a problem at the design stage. Yes, I know, sigh, another pointless discussion on designing a module, system, interface... Well yeah, that's exactly why you do them BEFORE the code is implemented, and not waste your time and other people's time x10.

1:35 Skill issue

4 eyes see more than 2

For most of the problems stated in the article, an experienced lead developer could step in and resolve the issues.

Be careful who your audience is when using the S. Ward.

Primagen: Stop Doing Code Reviews Also Primagen: "Yea, I leave nits everywhere"

This all sounds like rocket science to me, the current project at work can f up a remote db when _local testing_ If there were more than 2 or 3 of us on that project it would be crazy as hell

Somebody who doesn't know how to do code reviews, or do pull requests, use a linter, or work in a team for that matter, being opinionated but wrong. That's the TLDNR of this article.

read at 33s

Meh. Our code reviews have caught so many issue and generally improved the code substantially. I wouldn't ever give them up. Sure not all comments are valuable and some are nitpicky or bikeshedding, but we don't take code reviews to heart and leave it up to the PR owner to decide whether to apply a comment or to ignore them and whatever they choose is fine, as long as they acknowledge the comment. Either way, I find them very useful and beneficial in our team and would never want to give them up.

The volume on this video is so low...