Stop Doing Code Reviews
Вставка
- Опубліковано 30 січ 2025
- 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...
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.
Code reviews are essential when dealing with juniors or newcomers into a code base..
or you just pair. Much better option anyways
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.
Code reviews is for everyone, even if you have 20 years experience.
@@CyLvCompetetiv nobody got time for that
@@vanov88 that's fair
The author wouldn't say this if you'd seen some of the code that I've seen juniors make a PR for.
Junior here, I can say with confidence that I really need someone to review my code
I. Need. Feedback.
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.
@@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.
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
@@askolotus_prime I do this anyway. The two aren’t mutually exclusive.
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.
Comrade, we cannot smash capitalism if you keep gatekeeping your SLA!
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 😩
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
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.)
Racist. No way this is real, youre just mad at the offshoring.
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).
> 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
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
@@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.
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.
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.
There's no free ticket. You just mentioned in your comment
Came here to say something similarly. Search for Dragan Stepanović async code reviews.
Est-ce que votre code est écrit en Français?
@@Jabberwockybird grep -r "merde" ./src
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.
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...
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.
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.
sounds like the guy nit picking your project doesnt like when you nit pick him. I had one guy do that to me, then I opened like 20 minor thing son his PRs and he stopped lol
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.
Basically this
ua-cam.com/users/shorts-lsAOl5CekM
Payment method should change then
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
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.
With ping-pongs you can spend weeks on something merged in a few hours of pair programming
20 files is a big change? :0 oh no
@@rafasupronowicz196 except from now you've spent 2 devs to do the job of 1...
@@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.
@@incogneeto2418 "Complicated change ...."
And that's the OP's point: break down your work into small changes that can be easily comprehended and reviewed.
If you're doing pair programming you still need someone else to do the code review.
100%, even when the rest of the team did mob programming I caught several production errors
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.
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.
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.
@@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?
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.
@@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
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.
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?
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.
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?
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.
Coworker: This function seems too complicated for me.
Me: Skill issue.
*merges anyway*
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.
@@epiicSpooky TL;DR merge approved.
Coworker "I won't approve this until you make me understand it"
@@josevargas686 and that is why the "Merge without waiting for requirements to be met (bypass branch protections)" checkbox was invented 😅
Yeah, acting like a dick towards your teammates instead of proper reasoning is always a good way to improve team dynamics
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.
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.
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.
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..
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.
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.
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.
Agile/Scrum have an entire generation thinking they can get away and keep mentally sane without proper design and requirements. What a madness
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.
"That shit works when you own the project."
Someone has to "own" the project at a company too.
@@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
@@ytdlgandalf Redesigning stuff is part of software development anyway. It's not a huge ask.
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.
huh, remote or in-office?
That's so interesting. Is there generally at least one senior on each team?
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.
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.
@@MrMustachehead yeah, ofc, it's a must.
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.
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.
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.
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.
There is no such thing as pain free learning, the only thing that is pain fear is repeating answers that are feed to you.
@@allalphazerobeta8643 perhaps not pain but maybe failure. You definitely need to fail over and over to become great at something
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.
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~
What if I told you that the same benefits are possible, but real-time. That's literally mob programming: ua-cam.com/video/28S4CVkYhWA/v-deo.html
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.
yea except their feedback is "hey you really should rename this function to be a verb" or "I don't really like this list, use an enum instead just because I prefer enums"
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.
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.
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.
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.
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.
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.
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...
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.
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.
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.
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.
@@CyLvCompetetiv Oh, sweet summer child...
@@CyLvCompetetivAnd how do you propose you validate the juniors are now up to speed? Perhaps some sort of review? Of their code?
@@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.
@@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.
It helps to know if the code matches the ticket.
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.
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.
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.
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!
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. 😫
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
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.
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.
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.
Maybe they set a torch to their repo every quarter
"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
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.
Edit actually been a year lol
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 agree that not having docs on this is dumb. But I don't see an issue with having standards that are team specific.
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.
@@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")
@@rand0mtv660 Yes, so much this, and I said the same thing regarding enforcing the conventions with a linter if they are that important.
@@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?
Code reviews are only good if the reviewer knows what they are doing
Really? You mean doing a good job is only possible if you're good at the job? Gee, I never thought about that. Thx.
I feel like doing code reviews are super dumb for low impact work or changes, or for proof of concept projects. I had this one guy nit picking me on every line of a project that was literally a proof of concept. I was like "chill bro! this code isnt even used in production yet its just a demo project"
Near the end with your criticism of pair programming I realized something. I could never put my finger on why I dislike all the UA-cam “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.
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 ✌️😁
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 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.
Prime, at my job my manager assigns me the tickets, (the project manager btw not my actual manager) then I solve the ticket. but the project manager considers design docs part of the solution. not realizing that the feature is actually what matters. so he makes me reverse engineer the design docs that would match the solution I made. then we discuss the solution as a team despite me already having completed the work. and in this meeting we discuss the design documents. its completely f**ked
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.
Open PRs should be handled with a sense of urgency because they create a broad drag on productivity.
They might be blocking changes down the road, the longer they exist the more merge conflicts arise and they take up mental space.
Everybody is and should be busy in a company. That's the default situation. That's not a good reason to delay a code review.
@@EdmondDantèsDE All of that can and should be addressed with proper planning. It depends on the size of the team and the organisation, but where I work important PRs are not being blocked for long periods of time but unimportant ones certainly are. Also, there are plenty of things for people to work on that don't involve pumping out code.
I have been at companies where not just 1 person reviews, the entire team reviews every change. and I would have like people getting in to longwinded arguments in my code review. discussing full redesigns of the entire thing.. I am like "bro I am just adding a button chill... lol"
I clicked just because I dislike code review 🙃
Mandatory code review is like "do the job that I don't trust you to do without making mistakes."
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.
8:26 preferring changes with refactors sounds absolutely crazy to me.
Typical webdesigner mindset.
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
You should still be able to do that, just don't expect your hacking around to be accepted into the codebase (so don't raise a PR for it).
If you want to hack around willy-nilly, just make personal Open Source projects and flood them with commits :)
I’m camp ⛺️ “separate refactor PR”
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 :(
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
...thank Baptiste Barbieri, Antoine Guy, Maxence Zajdlic and Rafael S. Ward...
Let me guess, for reviewing the article.
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!
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.
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.
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.
I wanna do this kinda stuff for a living... what do?
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.
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 :|
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.
Comes with it's own problems and still needs a PR check imo.
For most of the problems stated in the article, an experienced lead developer could step in and resolve the issues.
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.
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.
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.
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!
For gods sake, Flip, take a limiter and add like 10db of gain, the video is two times quieter than LTT for example. UA-cam reports -4db on a random LTT video and -14db here. 10db is quite literally twice as loud, the scale is logarithmic. If it sounds too loud for you in Premiere or wherever just check the meters so you don't have to rely on your ears
Sounds fine to me. You sure it isn't just LTT absolutely blasting your ears?
As a side note, what would 0dB be?
For gods sake, Flip, keep up the cracking work - my sound is fine.
@@CottidaeSEA Digital dB is relative to 0, which is the loudest peak any digital audio format can do without distortion (32bit audio might be changing that, but that's relatively new). It is usually a good idea to keep at least -0.1 dB of headroom for codec related stuff (some might compress your audio in a way that makes it louder)
As to Primes video loudness - no, it is not fine and no, LTT is not blasting it too loud, youtube would lower their loudness automatically if they did that (the 100% / 100% in the stats for nerds thingy). Most videos i've checked real quick range between -6 and -0.1dB. The latter is on the Drumeo channel that is produced by professional audio engineers and it still has 100% loudness, so UA-cam did not lower it. Why? Because peak loudness != perceived loudness. Perceived loudness these days is measured in LUFS, a scale that first of all measures loudness on much lager time intervals (usually it takes the average of the entire recording) and takes into account that our ears perceive different frequencies with the same dB rating differently. AFAIK UA-cam lowers your levels if the audio is louder than 14LUFS (or somewhere in that range). This video is -28LUFS. That is WAY too quiet. That Drumeo video is -14.1 LUFS, which is exactly what you want and is again TWO TIMES louder that this one, LUFS is logarithmic too
Sorry for the rant, but I can't stand people that get paid to do this get audio levels this wrong. It's fine to see a difference of 2, 3, or even 5 db in some cases, but 10 is something I notice immediately and start to wonder if the editor even checked their levels
@@lolotronop That's interesting. Not sure exactly what to make of some of it, but I get the general idea.
I do think it's good if audio levels are kept equalized on any given platform, but I rarely find audio to be too quiet, mostly too loud, so it's also interesting that you have the opposite problem.
@@CottidaeSEA I've set my system levels to be perfectly fine on most videos I watch, not a small portion of which are made by people in the music or audio industries who know all that stuff way better than me and set their levels in their videos accordingly - at the platforms limit. So if something is two times quieter than that limit it is way too quiet for me, but no video on YT can get too loud. Maybe it just so happens that you mostly watch people who don't know about this and set their levels at whatever they feel is fine and most of the time it is quiet (as compared to YT's limit). Which is fine, but I wish more creators knew some of that theory because it would make for a better experience for everyone. Just imagine having the same loudness on any video you put on
As a side note - YT does not make videos louder automatically because they can't possibly do that reliably and distortion-free without modifying the original audio with limiting or compression and doing that without the creator controlling it is a horrible idea, it could ruin the audio making it sound completely "squashed" and "pumped" which is much worse that just forcing users to add a bit of volume on their device. But they can make stuff quieter because you don't have to do any kind of special processing and it can't cause any issues
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.
Wish it was that easy, they can't take rejection quite well when it comes to PRs.
There is a different between wrong design and redesign.
IF you're told to go off and do X, and you follow X to the tee, and then you open the PR, and someone tells you to redesign it because they weren't part of the initial design discussion, that's absolute BS.
If it does need a redesign, that should be done after.
The entire point of being agile is continuous improvement.
Even if the initial design is somehow flawed, you can't let perfect be the enemy of good.
Merge what you have, and do a redesign ticket.
If X is to be used by Y and Z and others immediately after you merge then your problem has grown 3x and let's be honest that redesign ticket is not going to get done instead it will be more bug or feature work. Going down this path is how you end up with a mountain of tech debt rather than just setting yourself up with solid foundations
What a trash article. Luckily Prime is able to bounce on it to make things interesting.
Does it count as a code review if I have chatGPT review the code it just wrote in another instance? (Asking for a friend)
How to get Knight Capped
We needed a code reviewer to block merging Imperial System.
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..
What is "java brain " means ?
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?
He had the conclusion in mind before writing the article. And so he wrote a great article with a shitty conclusion
"keep rebasing" is solved by using a RCS that allows files to be locked/checked out for change by a developer until the changes to that file are accepted.
1:35 Skill issue
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.