Stop Doing Code Reviews

Recorded live on twitch, GET IN
/ theprimeagen
Article: vgallet.github...
Become a backend engineer. Its my favorite site
boot.dev?promo=PRIMEYT
This is also the best way to support me is to support yourself becoming a better backend engineer.
MY MAIN YT CHANNEL: Has well edited engineering videos
/ theprimeagen
Discord
/ discord
Have something for me to read or react to?: / theprimeagenreact
Kinesis Advantage 360: bit.ly/Prime-K...
Hey I am sponsored by Turso, an edge database. I think they are pretty neet. Give them a try for free and if you want you can get a decent amount off (the free tier is the best (better than planetscale or any other))
turso.tech/dee...

[@spamviking8591]

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

[@vvan3711]

Junior here, I can say with confidence that I really need someone to review my code I. Need. Feedback.

[@rand0mtv660]

Yeah I agree. You cannot just let people go crazy on a codebase, it's just bad for the project. If you do that, everyone later will just be frustrated how horrible it is to work in that codebase. Only thing I can say as a person that does a lot of code review is that I sometimes get frustrated by how much time it can take me or how it can interrupt me during my coding, but other than that I think code reviews are quite a valuable tool if you don't focus on "function vs arrow function" type of debates in review and actually focus on the project domain and actual code implementation.

[@WildfireS1]

@@vvan3711Great self-awareness. Feedback is crucial in the early stages of a career, but it’s always important even later on. Being open to feedback is important. You’ll go far and rise to great heights no doubt.

[@askolotus_prime]

so just pair program with a junior for half and hour every day, it's easier to catch bad ideas or patterns early on than review dozen of classes and intricate logic afterwards

[@spamviking8591]

@@askolotus_prime I do this anyway. The two aren’t mutually exclusive.

[@silak33]

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.

[@rossbagley9015]

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.

[@isaacfife]

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.

[@codeman99-dev]

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!

[@quarteratom]

I see now that ThePrimeTime is just lazy, hacks together everything, then forces his teammates to fix his mistake. Gets away with it with just bullshitting.

[@dickheadrecs]

just ban nits…

[@Seedzification]

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

[@CyLvCompetetiv]

or you just pair. Much better option anyways

[@SPeeSimon]

I have seen "senior developers" adding changes that included bugs, did not do the thing it should do or were not "up to code" or in align with the technology. So i think they are always needed.

[@vanov88]

Code reviews is for everyone, even if you have 20 years experience.

[@Seedzification]

@@CyLvCompetetiv nobody got time for that

[@Seedzification]

@@vanov88 that's fair

[@seancooper5007]

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

[@dandogamer]

100%, even when the rest of the team did mob programming I caught several production errors

[@TheVideogamemaster9]

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.

[@alanhoff89]

Comrade, we cannot smash capitalism if you keep gatekeeping your SLA!

[@niko-pp]

lmao we have the exact same problem in my team, I'm thinking about create a lot more tests and strict rules in our pipelines, maybe it will help a little bit but I don't have time for that 😩

[@CyLvCompetetiv]

I mean at that point you re basically not a team. You are just a collection of people that are working on the same code base. Basically Open Source project where you cannot trust anybody, but instead you cannot trust any of your colleuges. So the "No Code Reviews" isnt the Problem here its a trust issue in your company

[@epiicSpooky]

This sounds like it supports the author's statement - you don't need code reviews, you need something even more hands-on. Pair program with them driving. (In general, I very strongly support code review & style guides. And don't always support pair programming. But I do think it's the fastest way to level up the team, as long as the one driving is generally not the more senior person.)

[@Snail641]

Racist. No way this is real, youre just mad at the offshoring.

[@lpprogrammingllc]

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).

[@sadboisibit]

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

[@epiicSpooky]

You're probably not wrong, but that's still the worst thing to do. If someone finds something difficult to follow, someone else will too, maybe your future self. And maybe with time it will be (re)figured out, but that's time wasted that could have been avoided with a quick refactor while context is still fresh. Though before refactoring, do find out what they found complicated, because by itself that is admittedly not a very useful comment. And in doing so maybe they can learn to comment better. Then upon next visit to the code by whoever, the next change will be faster and you have a net savings in time for feature development / bug fixing. Any review question hinting at not understanding a change, even if it's by someone very new should be taken seriously. Code is read more than written, and any reduction of reading effort has compounding benefit. And even if you don't change anything, at least you can help level-up the person asking the question.

[@sadboisibit]

@@epiicSpooky TL;DR merge approved.

[@josevargas686]

Coworker "I won't approve this until you make me understand it"

[@sadboisibit]

@@josevargas686 and that is why the "Merge without waiting for requirements to be met (bypass branch protections)" checkbox was invented 😅

[@bionic_batman]

Yeah, acting like a dick towards your teammates instead of proper reasoning is always a good way to improve team dynamics

[@WiseWeeabo]

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..

[@stubb1qaz]

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.

[@HeyPumpkin]

So very true. If the expectation of your company is that code will be deleted or entirely rewritten within a year or two, then yes, code reviews are going to be a lot less valuable for the company and could be skipped. But while code reviews might not be valuable for the company in that case, I would argue though that they're still valuable for employees. So if they're skipped, that's bad for the growth of the people working there. To not have your code scrutinised and be able to defend what you did and why, or otherwise learn how to do things better, and to miss out on what you learn just by experiencing the code review process, that's not good. It's also going to be pretty damn unrewarding long term to never write anything that stands the test of time. I would hate to work somewhere like that...

[@thekwoka4707]

Yup. I have been successfully freelancing since I started this career and for the vast majority of the work I've done, I've basically had no one more experienced do code reviews or poke holes. It was difficult to ensure I was progressing and getting better. The stuff worked, but I could be missing more dramatic design issues. I do self reviews and such, but it's very useful to have another human look it over and see if it makes sense.

[@Mastikator]

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.

[@SJHunter86]

I clicked just because I dislike code review 🙃

[@ErnaSolbergXXX]

Mandatory code review is like "do the job that I don't trust you to do without making mistakes."

[@ddomingo]

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.

[@errrzarrr]

There's no free ticket. You just mentioned in your comment

[@ChrisLasher]

Came here to say something similarly. Search for Dragan Stepanović async code reviews.

[@Jabberwockybird]

Est-ce que votre code est écrit en Français?

[@radfordmcawesome7947]

​@@Jabberwockybird grep -r "merde" ./src

[@wpelfeta]

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.

[@Salantor]

Problem is, it does not matter how many rules you can put in place, but whether or not the team can follow them and if the project situation even allows for that. Cause when everything is stable? Good, we can do that. The moment a hard deadline comes and problems start piling up? Suddenly we have to go around the rules, otherwise we will be late, features will not be done, bugs will pile up etc. Then add to that that someone might be on vacation, or sick leave, or has to juggle mandatory X hours of review with a pile of tasks and meetings and whatnot. Do you really want to make sure that code is properly reviewed? Don't put too many rules in place and don't overwork people. This is not a lack of guidelines problem, but a management one.

[@fanemanelistu9235]

@@Salantor"simply establishing a few guidelines" It's right there, in the OP's post, that he wants "a few", not "too many" rules. But why read-to-comprehend and reply to what's actually being said, right?

[@thekwoka4707]

Yeah, some of it is tough since humans. Style stuff should be handled with automated tooling so it's consistent and doesn't impact reviews. Ensuring the system doesn't spam everyone with "review requested" on every PR is good as well. Have multiple tasks so waiting for a PR review doesn't waste a ton of time.

[@Salantor]

@@fanemanelistu9235Yeah, why read-to-comprehend when you can quote a single sentence and ignore the rest, especially the first part of someone's answer? :P

[@nemesis-ad]

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.

[@rafasupronowicz196]

With ping-pongs you can spend weeks on something merged in a few hours of pair programming

[@SlavaEremenko]

20 files is a big change? :0 oh no

[@dandogamer]

@@rafasupronowicz196 except from now you've spent 2 devs to do the job of 1...

[@incogneeto2418]

​@@dandogamer code reviewing and pair programming both take 2 people. Just sync vs async. They both work for different things, it's never one option beats the other. Example: small simple change in familiar code - code review because it has low risk of back and forth. Complicated change or unfamiliar code - pair review because of high risk of back and forth. Send to a 3rd person for code review when done.

[@fanemanelistu9235]

@@incogneeto2418 "Complicated change ...." And that's the OP's point: break down your work into small changes that can be easily comprehended and reviewed.

[@EmperorRobin]

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.

[@JosifovGjorgi]

Basically this kzbin.info-lsAOl5CekM

[@azirious666]

Payment method should change then

[@billy818]

I worked with an ex Facebook dev on a react project and he made me a 10x better front end dev due to his pr feedback

[@quarteratom]

Typical webdesigner mindset.

[@thekwoka4707]

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.

[@Altrue]

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

[@DamnedVik]

> 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

[@aaryfn]

N^2 only matters if the input size grows. If we know the input size doesn't grow it's completely fine to do N^2 if it makes the code nicer

[@DamnedVik]

@@aaryfn N^2 case was a real situation from "senior" developer. And the code was objectively more complicated because of this. The developer was basically looping over an array, and then immediately had another nested for loop over the same array, and didn't use the value from the outer loop. It was essentially something like (java): for (String value in values) { values.stream.filter(v -> v.length() == 5).map(v -> v + "abc").toList(); } (obviously it was more complicated than that). Even if the input size won't grow, this is bad.

[@andrewbishop869]

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

[@fanemanelistu9235]

Really? You mean doing a good job is only possible if you're good at the job? Gee, I never thought about that. Thx.

[@rainerwahnsinn2150]

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.

[@CamembertDave]

It seems crazy to me that any "large change" could just show up out of nowhere in a PR. What kind of dev team doesn't communicate about the work they're doing?

[@sbqp3]

The problem is when you as a senior are asking yourself "what is this code even trying to achieve", "did this person consider these other approaches", "was this other person consulted on this", "did they test if this worked on platform x" etc etc. PRs drive you towards commenting on the code, but it completely misses the bigger picture aspect and chitchat that an in-person review would provide. PRs are not the place to start derailing. If communication happens primarily through PRs this situation can arise. And in that case, the problem is that communication happens too late in the process. Instead of getting a solution to a half understood problem thrown in your face, you need to make sure that everyone involved understands why it's being done and why it's being done in the way that's presented.

[@CaptainWumbo]

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.

[@Veptis]

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.

[@LC12345]

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.

[@Turalcar]

Maybe they set a torch to their repo every quarter

[@CamembertDave]

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.

[@ytdlgandalf]

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.

[@errrzarrr]

Agile/Scrum have an entire generation thinking they can get away and keep mentally sane without proper design and requirements. What a madness

[@shellderp]

at my work, we encourage a quick slack conversation before writing code, if you're touching code you aren't familiar with. For bigger projects, you send a design doc and get it reviewed by experts in the area.

[@isodoubIet]

"That shit works when you own the project." Someone has to "own" the project at a company too.

[@ytdlgandalf]

@@isodoubIet my point still stands, this won't work work in a company team. The owner is expected to have some process in place to not let colleagues fuck around and consequently veto their PRs into oblivion. As a last resort, sure, but again it's a smell

[@isodoubIet]

@@ytdlgandalf Redesigning stuff is part of software development anyway. It's not a huge ask.

[@CamembertDave]

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.

[@wadecodez]

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.

[@shellderp]

"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

[@rapzid3536]

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.

[@josevargas686]

You go ask the person to review, then you ask them again, the person tells you that they will do it soon and then they dont, you go ask someone else, well that other person reviews your code and the original one decides it was a good time to review too and now you got two people asking for conflicting changes. Your sanity starts to leave through the window...

[@keyser456]

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.

[@joe5head]

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?

[@Hersatz]

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.

[@CyLvCompetetiv]

Then just step up as a senior and start actually teaching and coaching the juniors in pairing/mobbing sessions. Teach them TDD and how to safely change code/refactore etc.

[@handlechar568]

​@@CyLvCompetetiv Oh, sweet summer child...

[@HeyPumpkin]

​@@CyLvCompetetivAnd how do you propose you validate the juniors are now up to speed? Perhaps some sort of review? Of their code?

[@CyLvCompetetiv]

@@HeyPumpkin If you pair with someone you get a much much much better understanding of their skillset than in a codereview.Instead of having to read their thought process in a code review, your pairing partner can litterly just tell you his though process right then and there.

[@HeyPumpkin]

@@CyLvCompetetiv Yeah, I'm not disputing that pair programming or collaborating on a problem with others is good to do sometimes. But do you really think you can just eliminate code reviews and leave juniors or inexperienced people to check in whatever code at the end of those alternatives? Even if it's just on the pair programming call, there should still be a final step where you review all of the changes to make sure you didn't make a mistake somewhere, or leave something out, or leave something in that you shouldn't have. That's what the code review is supposed to be anyway, so you should still be doing it, just without the traceability being as good as what you'd have if it was done through the more typical PR/review process My argument would be that you always should still have the code review process at the end of a piece of work. It's just that with what you're suggesting, the review should (should) be much quicker and smoother because everything should already be correct.

[@philippelarocque104]

Agreed with your conclusions. A quick 5-10 minute discussion is quite valuable, and means a single senior can do this for multiple other developers (or other developers with enough knowledge can do this for each other also). One thing I disagree with is how architecture is communicated. The bigger picture should be communicated as it helps find your bearings in the existing code, and helps create some expectation of where/how to add code. It's good to know if you are in a Controller/Service/Repository kind of deal or if you have more layers to deal with. Or if you are dealing with supposedly domain-driven code.

[@programaths]

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.

[@HyperionStudiosDE]

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.

[@allalphazerobeta8643]

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.

[@allalphazerobeta8643]

There is no such thing as pain free learning, the only thing that is pain fear is repeating answers that are feed to you.

[@dandogamer]

@@allalphazerobeta8643 perhaps not pain but maybe failure. You definitely need to fail over and over to become great at something

[@thekwoka4707]

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.

[@Georgggg]

Whats worst is blocking nature of code reviews. If you ever implement code reviews, make them non-blocking by default. If you don't like how people do their job, just fire them, stop dancing around.

[@mrpocock]

So what is the purpose of code review? It seems to be ceremony a lot of the time. Yet another hoop to jump through and meeting to attend.

[@cfehunter]

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.

[@StereoMyth1]

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~

[@PravinDahal]

What if I told you that the same benefits are possible, but real-time. That's literally mob programming: kzbin.info/www/bejne/aGm2ZXaMoL6bjaM

[@mwwhited]

This is probably written by a guy whose “totally revolutionary and amazing” code ended up being dumped on by everyone one else on his team.

[@0bzen22]

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.

[@isodoubIet]

This guy to me sounds like he's someone fairly junior who's experienced some frustration but doesn't quite get which frustration is a result of something wrong with the process and which frustration is just inherent. How would my people learn how to design systems if they never tried their hand at doing it themselves, having "pair programmed" every single feature?

[@chrishabgood8900]

It helps to know if the code matches the ticket.

[@rewrose2838]

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

[@NerdyDumbProductions]

I agree that not having docs on this is dumb. But I don't see an issue with having standards that are team specific.

[@rand0mtv660]

In that case, just check existing code in a project and follow that style. If you just randomly drop in a codebase and start randomly writing code that is way different than anything else inside that codebase already, that's on you in my opinion. Of course you can't go through all code and see everything and every little "code style" people used, but you can at least get a bit familiar with code and follow existing patterns. Of course, some of these like "arrow function vs function keyword" should be enforced with ESLint (or whatever linter is applicable to your language) so that people don't have to think about that during code review. Just agree on some of these rules, enforce them with ESLint and be done with it and focus on features and project domain during code reviews.

[@rewrose2838]

@@NerdyDumbProductions Team specific standards are great, but when a new guy is brought on, the standards are not relayed until he gives an MR, and its not like any of these are enforced by a linter either That is the part that bothers me, and especially so when these changes are not caught in the initial MRs, so now I had to make a lot of changes retroactively (in MRs that were merged previously with the usual "looks good to me, test once deployed")

[@rewrose2838]

@@rand0mtv660 Yes, so much this, and I said the same thing regarding enforcing the conventions with a linter if they are that important.

[@HeyPumpkin]

@@rewrose2838 Are you proposing to the team a solution for the linter, with the intention of doing the work to put that in place yourself? Or are you complaining that one doesn't exist, and expecting someone else to solve that problem for you?

[@Turalcar]

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

[@rauru8570]

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.

[@RobertSelberg]

This article sounds like it was written by David Farley from the channel @continuousdelivery. He has made a LOT of good videos in this subject. He also makes a lot of discussion style videos and I would live to see him on your show, like with Uncle Bob.

[@maromarcinko8632]

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 ✌️😁

[@cb73]

Near the end with your criticism of pair programming I realized something. I could never put my finger on why I dislike all the KZbin “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.

[@fahimzahir9587]

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

[@comodsuda]

ad. Redesign review: they happen to me sometimes and i hate them usually. Because my team sucks. Situation like you presented with nitpicking. I don't like your solution, because yes. Why are you using aspects here (used many times in a simmilar situation(logging) by author of the comment ) let's fire an event maybe. For the question is this worth doing because we agreed these are equal solutions actually - yeee, i'd go for an event, why? Because i like events. Month later guy created an aspect - asked why - because it fits good. Why not event? Because he likes aspects today, and this change needs to be released quickly. Fuck Also when I developed something and I need to introduce such change, structural etc. usually it's well tested i have something in mind - after switching to a new approach it's usually buggy at some point i covered before. eh..

[@sbqp3]

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.

[@potaetoupotautoe7939]

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.

[@cfhay]

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.

[@vorrnth8734]

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.

[@fanemanelistu9235]

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.

[@tetsi0815]

These type of blog posts where pair/mob programming is put above everything are most likely written by extroverts who need to suck the energy out of other people. This work style is extremely stressful and toxic for introverts who need quiet and peace to think and be productive.

[@spottedmahn]

I’m camp ⛺️ “separate refactor PR”

[@faizanqaiser4027]

Idk as a new comer to the field this stuff is just confusing. I just wanna be a good engineer i dont want to be part of a cult (clean code cult, functional programming cult etc) and every guy is saying his own different shit.

[@Voidroamer]

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..

[@n4bb12]

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.

[@pedroparamodelvalle6751]

Code review is aimportant. So much so even non-software engineers do similar stuff. Even Mechanical engineers review each other blueprints

[@HenrikoAlberton]

LGTM

[@datboi449]

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 :(

[@Andrumen01]

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!

[@bluladyfly]

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.

[@iflux8821]

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.

[@handlechar568]

huh, remote or in-office?

[@MrMustachehead]

That's so interesting. Is there generally at least one senior on each team?

[@crypticsailor]

I've worked in places like that and hate it. It's like swarming on agile. You get a lot more different types of issues with that.

[@iflux8821]

My team is remote but most others are in office with TVs and sofas ☺. I actually prefer remote due to Around and similar modern tools that allow you to take over control or simply draw on screen.

[@iflux8821]

@@MrMustachehead yeah, ofc, it's a must.

[@mudscuffer]

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

[@Stefan-qk8sw]

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

[@ReneHartmann]

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.

[@dandogamer]

Comes with it's own problems and still needs a PR check imo.

[@vladinosky]

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.

[@coolemur976]

1:35 Skill issue

[@migo70]

If their code was this gods gift that does not need the review in the first place there would be no comments in the actual PR.

[@dandogamer]

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

[@bonsairobo]

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.

[@bitbraindev]

4 eyes see more than 2

[@Zutraxi]

Sounds like the author isn't working with things that matter.

[@EnjoyCocaColaLight]

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

[@ANONAAAAAAAAA]

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.

[@patricknelson]

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. 😫

[@sethmclean8334]

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.

[@randyriegel8553]

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.

[@cheebadigga4092]

At work I have a merge request lying around and the reviewers didn't answer for like 3 months - and the reviewers I requested to do the code review wanted the change! Like okay thanks for the feedback, you suck by the way. Do I merge or close?! Guess I'm gonna merge myself lol

[@oblivion_2852]

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.

[@errrzarrr]

All the issues I heard from you come from Agile/Scrum mindset around coding rather than Software Engineering and code reviews itself. Anti documentation mindset: That's why you lack a style guideline and you have deal with that at the code review, the last possible moment, when anergy and moral is at the lowest and the most loud and outspoken reviewer seems to have some whims. Design Phobia: You keep thinking you can do good without doing design previously in the long term. Then comes the code review and lack of design comes to bite you. Decision phobia: You keep preaching about deferring decisions to the last posible moment and having "conversation placeholder". Well, you asked for it. These annoying lengthy code reviews are the placeholder for ALL of those deferred decisions. You asked for it, now you hate it. Take responsibility, make decisions on a timely manner. Anti architect: Somehow you thought talking trash against the architect was modern and smart because a non-tech scrum certified told you. But the architect used to clean up all this mess for you before they even appeared and made life easier for all of us.

[@jayshartzer844]

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.

[@KerchumA222]

The volume on this video is so low...

[@spyroninja]

Don't code review. Don't even compile. Just make changes and merge.

[@cidercreekranch]

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

[@georgehelyar]

Use the code review pyramid

[@stokedfool]

"non-coding architect"...

[@gabrielcrsaldanha]

We needed a code reviewer to block merging Imperial System.

[@ward7576]

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

[@juhjuhjuhjuhjuh]

How to get Knight Capped