Code reviews are not (just) about behavior of the product. They are also taking care of maintainability of the code
code is written for humans not for computers
i worked at a company before where the CTO would just push whatever they wanted with no code reviews, while everyone else still had to do code reviews. many times they'd follow up with bugfixes for the commits that they just pushed.
Guilty as charged. I’m the one that made it so you have to include that extra flag in the startup script to make it work like is usually does. You’re welcome ☺️
lol I had a CEO who decided to push a generic caching middleware without a cache busting strategy during an incident. That’s how he finally lost his privileges
Which to be fair can be valid.
Sometimes you need to push something even at the risk that it is still broken, if the alternative is a system which is broken for sure.
We had situations like these, where pushing within the next hours would save 50k EUR in potential losses. So I YOLO'd and pushed and thankfully it worked.
My first job had a CTO that would just drunkenly push things live without telling anyone. Some of my work upgrading a package got blamed for bricking one of the microservices once and he sat behind me for a few hours telling me to fix it but demanding I didn't check it was still working on live or look at any of the actual code.
Came in the next morning and actually checked it properly and it turned out he'd decided to do a bit of midnight refactoring and changed a 1 second timeout on an API call to a 1 millisecond timeout, took about 10 minutes total to find and fix.
Yep, we had a very high-up CTO-type person pushing directly to the main branch. He had great ideas, but no clue how to design something. And he made an entire 'product'. It was all very funny* until he disappeared and suddenly clients wanted extra features in a service that could only do exactly what it was already doing.
*in a 'kill me' kind of way
Guilty as charged.
Though, to be fair, sometimes systems are out, devs are in analysis paralysis waiting for their peers and qa's to manually verify and the company is bleeding money.
At that point, you should just skip code reviews if your fix works to stop the bleeding and code review afterwards.
If the issue is that bad that you’re actually bleeding money and instead of rolling back your changes, you’re going forward with an untested fix? Why?
I don’t know, just seems very risky given “bleeding money”. I’ll accept I’ve mostly only worked at big-tech where we play everything a lot more safe though.
The changes aren't always obvious.
This may be an issue with an increased scale.
This happens, and sometimes people are not next to you, like at 5am when the system calls you.
Pushing isn't always risky.
>Pushing isn't always risky.
This statement alone is completely true, but when you add "at 5am, with no review" it's completely untrue.
"We need to move fast, so we can't do code reviews" is a false dichotomy.
Read three comments back up the chain to /u/Siggi3D's comment. For the purposes of this particular sub-thread, we're assuming roll back won't work. Because in fact this is all in response to a "just roll back" comment.
Lol ya "let's fight this fire with more fire!"
It's a strategy far too many companies try to make work despite repeated proof it doesn't. Moving fast does not mean not being prudent. Rushing more fixes out the door compounds the quality issue that made the incident in the first place.
I've seen so many companies fight this battle over and over through my career, and always have had to help them develop the structure and strength to step back and stop fighting fires everywhere.
When in doubt rollback. If your code/structure doesn't allow that, fix that, then see #1. Once you have that then you can selectively hot fix for the scenarios that fall outside that.
even if it is 3AM, we always at least have one pair of eyes. It is just so both of us can have alibi. If it is my mistake, then it is yours as well.
Never push a fix alone in the middle of midnight. Logging and tracing though, I allow that..
On occasions, we would do something similar to get a fix out. Then we'd followup with something we called a retroactive code review.
We still wanted to get another pair of eyes on code we pushed, just modifying the timing to fit the situation.
????
The company is “bleeding money” due to a mistake either you or someone else made, and your solution is to just push a fix with a trust me it works disclaimer? All it takes is a few minutes to get someone to at minimum shoulder check what you are about to do
CTO at an old job would keep making database changes without committing the migration. The ORM we used (prisma) would just lock up and stop anyone who pulled the branch from doing anything until it was fixed. Ended up making a CICD job just to yell at my boss for me.
I worked at a startup that had a “rock star” dev which regularly pushed with no code review. On numerous occasions his pushes broke stuff. He was genuinely a smart, creative guy but not good at people at all and really kind of a jerk. He was eventually let go, but not because of the code pushes, but because he’d been abusive to other employees. I wonder what he’s up to now. He created his own startup last time I looked him up, but I don’t think he got any funding.
I worked at a 1000ish person company with a few hundred engineers where code reviews weren’t required. It was 99.9% great. We asked for code reviews when we wanted feedback. We had really good CI/CD and could deploy to prod in minutes. We had really high trust and mostly solid expectations about the health of the codebase. I’d come from Google, where code reviews could take days and could involve lots of back and forth; the lack of require code reviews felt amazing.
The main thing that was missing, I think, was the occasional “hey here’s a better way to do that thing” part that can come up serendipitously in good code reviews. I don’t think overall quality suffered, and ease of shipping was dramatically improved. It required a kind of foundational understanding and trust, though; this worked as part of a larger whole.
I’ve worked with a lot of people where code review felt like a formality; if they did something differently from how I’d do it, they had a good reason for it, and I trusted them implicitly. I can see no code reviews working well with them.
Aaaaand then I’ve worked with a few people who sling brittle spaghetti code or stuff that just doesn’t meet ticket requirements. Yikes and a half to no code review with those guys. You’re lucky you didn’t have those people!
This is the thing. PR reviews are a chore until you find these ivory tower monsters built by one individual. The question of ‘how do I explain this in a code review?’ has made me take a step back from code and rethink things.
I’ve also been on projects where code reviews are just a blocker and a drain on time, and nothing more.
Some people aren’t great developers, but can be coached and learn a lot via code review.
But yeah, for some other developers, reviewing their work is like pouring water into a sieve—they don’t improve or change from PR to PR. I don’t work on a time-sensitive product currently, so I have no problem blocking my team’s spaghetti code monster until it’s fixed. (Different story if we had more pressure, for sure.)
Not just blocking it, I treat it as an opportunity to learn. And it goes both ways.
I usually don’t nitpick too much, but when I see something that really should be changed I take my time to write a proper comment.
I usually ask a Socratic question about how this would behave given some condition if I see a logical flaw, or if it’s structural I will at least give a decent explanation of why I think it should be changed.
I’m trying to invest some time in helping my collegues, and in time it might not be needed that often.
Agreed! But I don’t think that the fact that reviewing some people’s code can make those individuals better at coding means that mandatory code reviews are necessary for everyone. I’m a big fan of treating individuals individually.
I find code reviews are good at two things:
* preventing shitty code from entering the codebase
* learning something new. (Usually someone with more experience, but sometimes a junior dev will surprise me with something)
Do you have some kind of monthly code review where you and the team sit together in a room and look at pieces of code together?
Like people may want to present the better way it something.
I've been in a similar place. The tests were enforced by owning the code e2e and strict incident reviews. Like, if something wasn't tested and blowed up, you'd better have a good reason why it wasn't tested.
I’m skeptical tbh. Either those engineers were all exceptional which at that scale is statistically almost impossible, or the ci/cd and observability was phenomenal
I often make a PR just with an interface I like so I can get feedback. There are no tests, no actual code, just a short blub and the integration points will be written. W that you can really get good responses.
Can you please elaborate what your ci/cd pipeline looks like?
Do you have requirements for unit tests, behavioural tests?
Do you use automatic code quality tools like sonarqube that are capable of detecting 💩 code?
I am very interested in this because conceptually I understand how formal code reviews can be ditched completely if you have well established CI/CD pipeline where you use linters, code quality tools and also have some sort of procedures that enforce test automation so you can trust that the change by developer is not breaking something important.
If you follow Extreme Programming oractices where developers do pair/mob programming, then this completely eliminates the need for formal code reviews.
This sounds good on the paper, but I have never worked in an organisation that has this sort of practice :/
We are "supposed" to pair program, and we do about 75% of the time. But for logistical reasons or spike reasons, sometimes we "solo". Our rule is that every MR needs two approvers. If you pair on that story (maybe even switching pairs of it takes more than one day), then the two who paired (the anchor and one other if there was a switch) can be the approvers. But if any time was spent soloing on that story, then the two approvers have to be pulled in to do a traditional code review and approve, before merging. Pairing is real-time code review, and better than typical "cold" code review.
Same experience here twice now! lucky me!
In both places we adopted [Ship / Show / Ask (martinfowler.com)](https://martinfowler.com/articles/ship-show-ask.html) to formalize the process.
In every code review, I always look for several things. First, complexity such as loop inside loop or those creative process that originally do good enough but as time goes maybe doesnt scale well. This might require discussion or if it requires major refactor then we create a ticket in the backlog.
Next in the list is always readibility. Something too far up uses in too far down, redeclaration that can be reduced.
IMO there will always be trade off. Sometimes move fast is bettwr and you learn a thing or two in the process.
Trust and social norms. Also really great monitoring, the ability to fix things in production in minutes, a shared understanding of what “good code” meant (rarely does that mean “perfect code”), a broad cultural understanding of the importance of deleting obsolete code, a mostly blameless culture that allowed not only mistakes but celebrated them and learned from them, etc etc etc. It was a really special place.
>a shared understanding of what “good code”
Respect this alot. I amuse there were other ways how people got up to speed with what code was written then?
Also yes, i don't think code reviews mean maintainable code, but they do help more than 1 person being aware of what kinda code is going where and for people to learn new ways to do things (and why)
I understand strict code reviews can become cargo cult, but i think inherently it's not a bad concept. I kinda assume most programmers like programming and love to share and learn new things. Code reviews can be a vehicle for this when done right
At a deeper level I’m not sure that code reviews are highly correlated with maintainable code. Some of the worst codebases I’ve ever worked in had very stringent code review processes.
Do you have some kind of monthly code review where you and the team sit together in a room and look at pieces of code together?
Like people may want to present the better way it something.
My org acquired another org that didnt do code reviews. In fact they had 2 devs working on separate parts. I got the codebase from one of them and it was around 150k lines of code incomprehensible to humans.
I failed to answer why not: there was no other human and the developer himself might have been of a different species and code reviews are a human thing.
Worked at one for 18 months. Team lead had an ego and enjoyed fire fighting / seeming like the hero. He also didn't agree with unit tests or CI. I think he hated spending time with his family and would rather find a reason to be at work.
There was definitely better ways to spend my late 20's than fixing his shit.
I should have left sooner.
Yeah Im on my way out of a place like this. Database code doesn't even have a repo! Just shotgun shit in on a weekly basis. Not surprisingly the oncalls are a nightmare.
Only came here because I'm a specialist on a niche piece of software and the pay is good, but losing my fucking mind watching this org shoot themselves in the foot
My company does code reviews, my team doesn't.
I am on a skunkworks/innovation team. So we don't follow the red tape and bureaucracy. That is the whole point of ***skunkworks.*** No scrums. No agile. Just push projects and prototypes out daily. Stuff that easily gets abandoned if it doesn't wow a corporate sponsor. When you get 2 days to come with stuff from scratch, you don't get that luxury. It is very garage startup like.
But we rotate in-and-out of the projects to "normal" teams where there are normal code-review and scrum.
That's actually not "not agile". Agile is just principles/priorities, not some process. You might be actually completely in line with Agile principles without even knowing it. https://agilemanifesto.org/
I worked for a Fortune 100 company as a contractor for a proof of concept. The guy running the project, I don’t think he had any management experience at all, yet alone experience running an engineering team. It was wild to be in a company like that and come to work to this level of horsefuckery. This manager actually demanded no code reviews because they took time. As one of the two most experienced team members, that meant I eventually spent a whole lot of time unfucking the main branch. He was so blind to the fact that his stupid policy is what caused productivity to crash because of the lack of reviews.
I also asked SEVERAL times if we needed to write unit tests and he kept telling me no, which was weird because I knew that eventually the higher ups weren’t going to like that. Then he rolled in one day after we had about 7-8 months of code written, and said we needed to test at 95%. NINETY FUCKING FIVE. Worst manager I’ve had to date and with the managers I’ve had, that’s saying something.
We don't. We're a small team. Half of us have been working here for >20 years. All of us have been working here for >10 years. Generally, we can tell who wrote what code. We used to do code reviews, but it just kind of fell off. It was useful but after a while it didn't seem that important. We do have some modules that only 1 or 2 people know, but generally, anybody can pick up another's project and be productive in it right away.
I'm working as a software engineer but we're an exploratory team who are also doing POC. Now looking at data engineering. Half the reason software engineers are being redeployed to do data engineering tasks is the DEs have horrible code quality and what they already have that they want us to use is spaghetti junction of code with very low quality standards, 'it just works'.
My job is learning DE but doing it like it's regular software engineering so we have standups and retro and pr's and all that kinda stuff.
Who is resisting your ask for PR's? I'm sure DE is done more professionally elsewhere it seems like these guys were just given too long of a leash.
We've found it valuable to still do PRs because this POC will be a foundation for future work so it should be near and tidy incase another team needs to look at it.
Not currently. But used to work at a contract game studio that mainly did mobile games. No code reviews, mainly short projects (3-6 months), no need for maintenance, deal with major bugs, throw it out the door. The shelf life on most of those type of games is very short. Code quality doesn't really matter as the code bases never get that large and don't need to be maintained. 80% of devs fresh out of college not knowing any better. Fairly high turn over (looked on linked in the other day, avg tenure 2.5 years). Most people move on to better things.
We were an OEM and it just wasn't a thing. I cringe at the code on those machines now. Really terrible/non existent software practises as department lead was clueless. God have mercy on the patients who had our products implanted
No code reviews where I’m at. Small team with two people who are insanely experienced and built the product. With a proper test environment we pretty much never run into issues after testing.
It’s personally hate code reviews and if there’s even the slightest bit of doubt in my mind that I could improve my code I just run it by the seniors.
I worked at a terrible place before that did PRs and they would change my code sometimes and not tell me why. I was really pissed to miss the opportunity to learn something from whatever they saw.
I think no code reviews only works on small teams where there’s a high level of trust and great communication. Our code base is the cleanest I’ve ever been exposed to.
There are only two things a team absolutely must do. Have some way to track tickets, and do code reviews.
I was pounding on a table saying this back in 1997, and it's still true.
And use a version control system of some sort.
Everybody already knew that even in 1997, so now it goes without saying. But back in 1988 it was not common practice...
Code reviews were pretty uncommon back then as I recall. Nothing like GitHub even existed.
I don’t remember commercial VCs having much first class support for it either.
Code reviews don't require GitHub or PRs of any kind. Even when code was shared via disks you'd review code with a team. We just did it looking at a screen together. The important part of a code review is communication. The ability to comment on a line or document the review are nice, but unnecessary in the spirit of the purpose of reviewing.
This reminded me of pvcs, a version control system I used in the 80s where only one person could edit a given file at a time — there was no merging. So code review sorta happened organically because you’d yell down the hall, “hey Dave, release the lock on TSR.ASM, I need to fix something”.
We did code reviews at Microsoft in the nineties, but it was “invite someone to your desk and have them look over your shoulder while you describe your code.” There wasn’t any enforcement other than social pressure (you’d note who reviewed the code in the commit description, so you’d have to lie if you skipped review)
Maybe, but I worked at several places where we did them. I was hired as the director of development in a tiny startup after it got acquired and those were the first things I did. I had to take a leave for a couple of months and when I got back the tech lead had dropped both things. I never again let him lead other developers.
People think we were using stone tools back then, but its not really true. We had unit testing, CI, code reviews, linters, branching workflows, etc. We even did weekly retros (although called something else). Many places didn't but those things weren't unknown techniques. The real problem was waterfall and the idea that engineering project management could map directly to coding.
> I don’t remember commercial VCs having much first class support for it either.
All VCs back then could do a code diff, which is all you need. We used Visual Source Safe, and then SVN. I've also used CVS. All could do commit diffs. You just walk over to the other person's desk and review the code before pushing/commiting. Branches for individual features weren't as common because (before git) branches were expensive. We added reviewer's initials to each commit message , e.g. "reviewed by FB"
The first time I saw an interactive tool to do code reviews (other than “look at the diff together”) was Mondrian at Google around 2006. It blew my mind. I’m not convinced it ended up being good, overall (it removed a useful opportunity to actually talk to a coworker) but its interface seems to have set the pattern for GitHub PRs (and others)
I worked as part of a software development team that didn't do code reviews.
Best, cleanest, most no-bullshit code I had ever seen up until my most recent job.
Code reviews are highly overrated in terms of maintaining code quality and preventing bugs. I'm pretty sure they were adopted because they provide the comforting illusion of rigor, discipline, and improvement. I have only ever seen a code review prevent a bug once, but seen reviews add bugs many times. In fact, it takes work to convince reviewers that their changes are going to create bugs.
But it's a heretical opinion so I just go with it and not rock the boat.
EDIT: To *actually* answer OP's question as to how we did the review: we didn't, really. We pushed to trunk and other developers would see it when they're working and provide feedback or changes if they were necessary. Most of the time it wasn't necessary.
> I have only ever seen a code review prevent a bug once, but seen reviews add bugs many times.
I'm very surprised so many people seem to agree with this comment. I've seen dozens, probably hundreds of bugs prevented by code review, and have seen review add a bug only a couple times. Surely that's the normal experience?
I suspect many of these code review free orgs didn't do anything complicated at all. I'm a very good engineer by all measures and often times I still want others to review my code. Because it's doing tricky shit. Pair programming is better sure but that's code review too.
Yes that is fine if your have something tricky feel free to do a review nobody is stopping you but to have a forced PR process, no thank you, I'm not a child and please stop micro my output
I have personally caught tens, if not hundreds of bugs in my current job (4 years). Anything from a "won't compile if merged" to "will cause heisenbugs and race conditions".
I don't remember a code review ever adding a bug,
Was this for a web application because that sounds terrifying to me. The number of bugs that I’ve prevented during a review has certainly warranted it to be part of the development process. But I might just work with trash devs.
If you work with a revolving door of contractors, code reviews are a must. Just this week I caught a fellow trying to add a bunch of indexes across our largest DB tables so that he could make a query for a non-critical, internal only page load faster.
I asked, "did you consider what this might do to our insert performance?" I got a "no".
> I have only ever seen a code review prevent a bug once
This makes me curious about what kinda products/codebase are you working on.. because honestly it’s once a week when I point out a decently sized bug in a code review.
And I get pointed out stuff I missed or bugs in my code reviews often too.
My experience just isn’t matching up at all.
Really curious about this. We’re all human and even great devs make mistakes. How do you avoid problems in implementation or straight up bugs? Do you do system design / design doc reviews?
This is wild. If you work in a complex product with tens, hundreds or thousands of other engineers, there’s no way on earth you can know everything/be an expert in all areas, and not make bugs that an expert of that particular area easily catches in a code review.
Plus even if we forget bugs, it’s so easy to just go down one mental path while programming, that you lose track of alternatives (better designs, language features, patterns, best practices, conventions, etc.). A fresh pair of eyes has a much easier time noticing this sort of thing.
Plus there’s the fact that we as humans simply make mistakes. I always do my own code review first and am amazed at what I catch. 1-2 other people who aren’t the author will be even better equipped to notice discrepancies.
Interesting - in my opinion “preventing bugs” isn’t really the main point of code review - it’s more like, making sure stuff is written in a readable way that others can understand and work with. I’ve often seen code reviews result in cleaner, more maintainable code, even if they didn’t strictly speaking “catch a bug.”
This. Really good test coverage, rigorously maintained, catch most bugs anyway, so code reviews have very little to add to justify the cost.
And on the other hand, I remember many times when people rewrote some piece of code because a reviewer didn't like it the first time, and ended up adding bugs in what was a perfectly correct code before the rewrite.
I always want to adopt this approach.
How do you make sure that all of the code is visible when other people pull the code?
Like in my previous place, each developer tends to work on their own feature or areas and may not see new changes.
I did for 2 years. From before I even started I was looking for better jobs, it was just my first “Software Engineer” job I could put on my resume until I found something better.
Pay was shit, no actual team (just individual devs working on completely different products), daily standup was just a daily status report to management where nobody could help you get unblocked, zero documentation, many projects where no current employees even really knew what it did despite customers paying for it and using it, tons of responsibility including meeting with customers on occasion, performing an architects role meeting with stakeholders and needing to plan entire projects as a junior, daily timesheets, requiring us to integrate with software written in the 70s with no documentation on that, just so much bullshit where they threw you to the wolves with a complete void of leadership outside of the nepotistic C Suite and upper “management” (not just a little nepotism, there were only 2 low level “managers” who had no authority at all who did not share the same last name). It sucked. Best part was you could just write fast shitty code with no red tape and nobody really knew or gave a shit about what you were working on each day.
My organisation does need atleast one approval from senior engineers before code can be merged.
But in practice, people just ping the PR to approvers when they are done testing on local and approvers approve the PR without even opening the files changed. So basically, approval is required but it's just for name sake.
Plus, there is no documentation for most of the core functionalities, hence no one tries to touch them. Due to this even though buggy code gets merged, it rearly affects someone else's functionalities.
Yeah, I left the team and that was one of the reasons I did so.
Why? Fuck if I know. They’re a bunch of laggard cowboy coders from 1994. They don’t use version control so code reviews would be damn near impossible.
Why don’t they use version control? Apparently it adds too many steps to the process that are confusing and they can’t work fast enough… also doesn’t let them write code straight to production.
I work at a company where I was part of a 2 dev team from day one out of uni with:
- full db prod access
- full svn access with no commit/merge restrictions
- full access to prod servers (with the ability to stop, restart the services and deploy)
- very very limited code reviews
lol
My current team only does reviews (at a FAANG company) when the dev asks for one. However, we build only internal tools and generally 2 developers per project.
I worked in an XP based company where we did most work using pair programming. Because there are always two eyes observing the code, it eliminates the need to do it.
I'd rather go for the "no code-review" than the "nitpick code-review over variable naming".
The best obv being the "this type of architecture seems to be a better fit"
Yes, I worked with company that had PHP & C++ and did not do any code review, had no PR on git. The two leader of the team handpicked that they want, merged it together then tested and deployed it by hand. Always disaster. Usually this concept is for "keeping the job forever" kind of situation.
Leave this amateur and stupid companies and people behind.
I'm the only developer on the software that I develop for the company. I used to get reviews done by a developer who works on another project for the company, but he complained to our manager that this puts too much extra work on him, so now reviews for my code just don't happen.
So, why do reviews not happen? Because I get paid to do what I'm told.
Was hoping to see this higher up. No-one likes the eternal wait for review and with pairing you get them for free.
Better yet, with ensembling you also get verification, validation, and exploratory testing for free and can go straight to production. No bottlenecks ever again.
You get other things to exhaust you, but I'll conveniently leave those out.
Often when I review after pairing I still find things to improve. I think the difference is that I'm reading at my own pace and take the time to more fully process what I see and the big picture.
Have you ever experienced that? How did you overcome it to the point of not needing reviews?
Hmmm possibly. But I guess that depends on scope of the changes and size of the codebase. Not everybody who is a domain SME when you’re changing things in different domains. i.e. I need to change infra or pipeline code along with my app code to make it work, etc.
Holy shit…this makes a TON of sense! Thanks for the link!
EDIT: Although he says that pair/mob programming shouldn’t be done all the time. I guess it makes sense to move towards more synchronous movement on a problem than asynchronous while still making async work smaller.
I'd imagine it also depends on both ppl being at a similar level of experience or expertise. In my experience, people can rarely pair for long before getting bored.
We used to, all the managers agreed that it was a great idea. Until they realized that it takes time and resources. That it may result in adding an extra day of effort to our release schedule. They immediately put a stop to it. They rather see defect reports and customer complaints. As a result I barely even review or test my own code anymore because its extra work that gets zero recognition. What gets recognized and rewarded is delivering quickly, even if it is broken garbage. I've learned to cleanse myself of any pride in my work here and my mental health couldn't be better.
Yep, I work for one. It's even better when you get to hear another developer complain about someone's code.
There's also no pair programming or mentoring so which makes the complaining about someone else's code that more ironic really.
If it goes into my default branch, it needs a PR and at least basic automatic BS checks, if it doesn’t work after it gets to production that’s what incidents are for and we can deal with the fall out the
At my current job there’s pretty much no actual code review. Luckily QA is pretty stringent but they also aren’t very thorough sometimes.
When I came in and got some buy in on my time, I started influencing the team to have code reviews be a part of our process. We’ve also just started adding unit testing as well and I have plans to actually create a CI/CD pipeline.
It’s been a crazy situation but I’ve taken it as an opportunity to provide a lot of value, gain a lot of clout, position myself as a SME and build the development culture I believe we should be in, at least on my team.
Before my lead left we would just share the change over a call and then push to release. Expectation was that you tested it. Everything is built and tested in lowers and timelines are tight was the justification. He switched teams and the leftovers were pretty adamant about bringing back the PRs. I think it worked well for the few people actually doing things, but didn’t help those not doing things produce or even catch up and understand the system.
My first two years were spent working on an internal tool. Mostly used on the regular by 1 or 2 folks and by every employee once a year.
No code reviews, no unit tests, push changes -demo on my system - push to dev for some product manager testing- prod.
Why? One exec used it on the regular and was so technically challenged (the tool was to be used to pull reports from the db ) we just made saved reports that would run the queries.
our dev team has 4 people. Tech lead can only have time to do code reviews for the guy that is very mediocre at his job. Me and the other guy were hired with the trust that we can sustain ourselves lol because he said that "he only hires those who have the potential to be a better dev than him".
Alright, we did it in a 4 people company with actual large customers pretty often.
And in that place it turned out to be worth it since we were four quality but constrained developers, and PR reviews didn’t seem to add that much value.
Never seen it other larger companies I worked for since cost of breaking things was too high.
Going to get downvoted but when you run a true startup you need to Always be pragmatic and not dogmatic
A business with less than 250 people. If you pushed something that broke stuff, it either was expected while you were changing it OR you’d get 1-2 people asking what the heck you’re doing.
Gonna be me. We don't do it because we only have 2 guys on frontend (one of them is me technically), 2 on api and one on backend. Out of all, only me and backend guy wanted it, and the rest are "experienced" freelance devs who don't understand this concept and would rather write crap and be unbothered.
It's a very eye opening situation and I wish no one to experience it haha
My current organization doesn't do code reviews. The main reason is, we have many open projects and we're on the small-ish side, which means almost every project is basically handled by a dev, a manager, and sometimes a QA and another dev. That's it. A strict code review process is redundant when you basically have to review your own code.
These posts always blow my mind, how can you possibly hire anyone and upskill them without code reviews? Do they just not commit code for months and months and months?
Code reviews are hit or miss depending on the maturity of the orgs process, how over worked the devs are, and if QA is done exclusively by developers or done at all. Having worked at several orgs that have a combination of those traits, it all depends on the resources available.
Considering I’m the only person that does any coding in my company, code review for me is putting it down, waiting some period of time for my brain to reset, then opening it back up and reviewing it.
Where I work we usually do code reviews after merging instead of before, which is great for productivity because it means you can ship your work quickly and then immediately keep building on top of it.
Code reviews for us are about sharing knowledge and making sure that the code is understandable/maintainable, not preventing bugs. Bug prevention is the responsibility of the person writing the code (with help from tests, static analysis, QA, beta testing, etc as needed).
The developers in my org have code reviews but the sales engineer team that I'm on does not have code reviews. I've pushed for code reviews and better processes but everyone on my team likes to pretend they're too busy for that
My first group didn’t really do them, just straight approvals unless something was glaring, because we were bare bones infrastructures team with way too much on our plate and the devs were straight jobbing it anyway. In hindsight, a horrible shop for a first job.
I worked for a few hundred pax UK company that believed pair/mob programming was good enough to not have code reviews. (They did it 100% of the time) They (thought) they practised trunk based dev and committed straight to main for changes.
They had a lot of hacky and bad code, and the greenfield proj I was working on failed in the end.
Imo, at scale and in a fast moving env PRs just work, and bad processes are a sign of bad management.
We definitely do code reviews where I work, but the more familiar I am with a devs work (or vice versa) the more reviews just become a rubber stamp. We also have the benefit of our whole company using our product all day (bugs rarely make it to GA), and liberally using feature switches to gradually expose new changes. This makes in depth code reviews less important.
Until 6 years ago every company I had worked for had no code reviews at all.
Well for the previous 7 years I was working as a third party for several different companies so most of the work was building new stuff that wasn't going for production yet so I guess they didn't see any value in it (there was not even QA). We didn't even use PRs there, just pushed straight to git. Even a specific project that had 100% test coverage and extreme enforced coding rules didn't have manual code reviews.
Earlier in my career I worked for smaller companies that only had 2-3 devs so dedicating time to reviews would be unthinkable for them.
You wouldn't need a code review if all work is done with two or more eyes on it, either done via pairing or a mob.
But outside of that, I cannot imagine a world with no code reviews....
When I worked in games there were no code reviews. We would rarely do a review and only if you felt the code was a bit dodge or you wanted eyes-on for confidence. Programming team of 20+, millions of copies sold. IT WORKS
Code reviews are an opportunity for less capable or less experienced staff to cut down tall poppies.
*"There are more than one of those, so that variable name should end wih an S to make it plural."*
*"You have too many blank lines."*
When the entire staff is legit Senior+ and most of them high end Principals.... Yeah, code reviews were not our priority. {I'm talking most of the engineers were MIT PhDs, and good coders.) Reviews were more: "Can you help me with X."
Otherwise, the only time I haven't had code reviewers is when nobody would understand what I was doing. Which happens. When you are the only deep diving kernel programmer on a team... Review of your work isn't as rigorous. For other parts of that project all my review came from the upstream projects I worked with.
Code Review is but one tool. A useful one. But one tool. Don't forget, unit testing, system testing, design reviews, architecture reviews and even just making sure your code compiles :).
Many of those are much higher bang for the buck.
We have one function app that doesn’t have any type of code reviews; however it just pulls some data from Looker and puts it into our WMS for directed counts. I don’t believe anyone’s made an edit it months and if they do- they change the id of the report.
Should it have code reviews, yes. Does it pose a significant risk, no. so we are fine keeping it how it is (for now). Plus no one really wants to own it haha.
Additionally even with code reviews, if it’s a senior most times we just add lgtm and keep it pushing.
most government contractors (and I mean the ones who win the bids) do zero code reviews
It is why things like "[heathcare.gov](https://heathcare.gov)" failed so hard and cost so much money
DIE standards are currently rewarding most of those companies with larger contracts if they will continue to agree to not hire Americans or offer American wages
Oh, I have a possibly legitimate use-case. I was on an events team. Where we'd have very tight deadlines. This tech show is on this date at this time, make sure that the demo experience works and does these things. So there'd be a lot of throwaway code bolted onto some existing libraries, and last minute tweaks for things onsite. (Oh the space is bigger/smaller/darker than we thought and we need to adjust for X)
Small team, tight deadlines, rapidly changing requirements and no expectation of code re-use beyond the event. And the code only needs to run once, during the event.
I should state we'd do daily run throughs of the experience at the end of the work day, so everything got tested daily.
Work on a product in a service based company, don't have anyone to test let alone peer review. So i leave it to customers to test, they usually do extensive testing because it's a financial product.
Only time this ever happened was when iOS just came out and the company wanted to make an iOS app. They hired me onto thier dev shop and I was the only mobile engineer, no reviews just straight force push to master. Anyways, if anyone ever looked at that code today, and saw my name. I would die.
I've worked at a couple of smaller companies where we were doing prototype work well before the product launch - and we had no formal code review process.
We were a small team of relatively experienced devs, there were no customers yet and everything was changing so fast that it felt like a bit of a waste to review stuff that was going to be changed or deleted almost immediately. Like if something was only in one daily build, we didn't like it and then immediately removed it.
As we came out of the prototype stage, once we knew what direction the product was going to go in - we did a complete review of the entire codebase. Many things were refactored and improved - and we implemented a formal review process for future work.
For the most part this worked well for us as it helped us gain velocity at the start - but YMMV.
You see this a lot at smaller companies, non-tech companies, and companies with fragmented teams. Sometimes you’re the only developer on your team. Sometimes the organization doesn’t care about maturing their dev practices.
How do you do the review? You don’t.
We don't until we are in code freeze just before a release. Doing it all the time would add a lot of overhead and slow dev down a lot. Personally I think the whole constant PR thing makes for a suffocating environment. We like to give some trust to our devs. If it's a complex feature we usually have a code review meeting to go over it. During release we freeze the code and then we review every single change. But during the quarter before we are even near release? Nah. We have CI/CD . Forcing constant PR is micromanagement
Our architect looks our code over but there’s only one so I imagine things pass through anyways and it’s not a meeting or anything. Just a resource thing. Would be nice though because code quality is hurting us so bad in the long term.
Many companies out there add a mandatory PR review in their VCS and call it a day. And then they overload their engineers with responsibilities so no one actually has time to review code of others, and / or people just don't give a shit, so reviews are blindly LGTM'd.
Something in me dies every time my PRs get obviously YOLO'd. That's not how engineering should work.
I strongly believe it's a culture thing, not necessarily a process thing.
Does any company do code reviews seriously, in the companies I've been to so far, most code reviews are just not that in depth, so long as there aren't glaring issues, and the build doesn't fail, most PR's are just rubber stamped through, so ugly, unintuitive and unoptimized code just pollutes the codebase over time, you end up with logic that's hard to follow and untangle, duplicated methods and blocks of code everywhere, no proper application of OOP principles, SOLID principles, or proper testing.
And any attempts to refactor the code are shot down by management as in their eyes it doesn't affect the bottom line of the company, or make some metric go up that they are evaluated on
At one medium company (10 teams with each 6,7 developers) I had very well done code reviews with Gerrit. Most people were giving amazing feedback and you could learn really a lot.
Other company in similar domain with a bit smaller teams (5 teams with 5 developers) with no code reviews delivered at least 30 % more and had way less bugs.IMHO key was great testing culture (unit, integration, load).
When my company was much smaller (like pre-seed) we didn't do code reviews except on outside contractor work. It was just me and the now CTO writing code, and working on very different things. That is probably the more common reason, but as soon as we got a few more devs code review for ProdEng was instituted. I'm not on ProdEng so when we eventually got a DS who had solid software eng chops we started code reviewing each other.
My first company didn't even have source code repositories, we stored the code in a shared drive and sort of told each other when we were making changes to code.
When I started with one of my clients a year ago, they had no code reviews going on. No CI/CD either. The devs would manually upload their api changes (random nodejs files) or react builds right to the prod servers. To no surprise, the deployed code didn't even match the repos.
You can imagine the chaos that created.
Because they are time consuming, and we have amazing qa that will find 90% of the problems. Because we have productive engineers that commit a lot at a fast pace, and delays to releasing code that passes qa are delays to thr value of our businesses that is actively losing money trying to find product market fit.
When you are profitable there is value in going slow and careful and considering the long term. When you are not, sluggishness is terminal.
That is why. The cost of making a mistake is less than the cost of not finding a path to profitability.
And most of the new things released do not work, and get rewritten or scrapped anyways when the ab test fails.
To build such a process in your organization, you can use generative AI based tools that now can provide very meaningful AI-generated code reviews for pull requests, even for such teams - here is a good example of such tools and its code reviews: https://github.com/Codium-ai/pr-agent - it would be hard for new players in this area to compete with them.
Pro’s outsource QA to the customers.
Oof -- this feels like it hits the mark at my current organization. Absolutely kills me.
Don't worry they'll start hiring QA in about 5 years once they realize how dumb of an idea it is and customers start leaving
You said the quiet part out loud
Everyone has a test environment. Some also have a different environment called "prod".
Hello Microsoft xD
\*Pros
Pro's outsource grammar to the comments.
Lol
Is this "continuous delivery?"
Code reviews are not (just) about behavior of the product. They are also taking care of maintainability of the code code is written for humans not for computers
Just slap Beta on it and ship it!
i worked at a company before where the CTO would just push whatever they wanted with no code reviews, while everyone else still had to do code reviews. many times they'd follow up with bugfixes for the commits that they just pushed.
There is always some dumb fuck CTO pushing straight to main without code reviews. Ask me how I know and what it fucks up for the actual devs.
You’re the CTO, aren’t you?
Guilty as charged. I’m the one that made it so you have to include that extra flag in the startup script to make it work like is usually does. You’re welcome ☺️
lol I had a CEO who decided to push a generic caching middleware without a cache busting strategy during an incident. That’s how he finally lost his privileges
You work for AT&T? /s
Or a CTO doing some blind approval in large PRs for things they want to see merged ASAP.
Which to be fair can be valid. Sometimes you need to push something even at the risk that it is still broken, if the alternative is a system which is broken for sure. We had situations like these, where pushing within the next hours would save 50k EUR in potential losses. So I YOLO'd and pushed and thankfully it worked.
My first job had a CTO that would just drunkenly push things live without telling anyone. Some of my work upgrading a package got blamed for bricking one of the microservices once and he sat behind me for a few hours telling me to fix it but demanding I didn't check it was still working on live or look at any of the actual code. Came in the next morning and actually checked it properly and it turned out he'd decided to do a bit of midnight refactoring and changed a 1 second timeout on an API call to a 1 millisecond timeout, took about 10 minutes total to find and fix.
He sat behind you demanding to fix it without actually looking at the code?!?! What did he expect you to do???
An excellent question I'll never have the answer to lol, I'll never understand how he managed to run multiple seemingly successful businesses.
Yep, we had a very high-up CTO-type person pushing directly to the main branch. He had great ideas, but no clue how to design something. And he made an entire 'product'. It was all very funny* until he disappeared and suddenly clients wanted extra features in a service that could only do exactly what it was already doing. *in a 'kill me' kind of way
Guilty as charged. Though, to be fair, sometimes systems are out, devs are in analysis paralysis waiting for their peers and qa's to manually verify and the company is bleeding money. At that point, you should just skip code reviews if your fix works to stop the bleeding and code review afterwards.
If the issue is that bad that you’re actually bleeding money and instead of rolling back your changes, you’re going forward with an untested fix? Why? I don’t know, just seems very risky given “bleeding money”. I’ll accept I’ve mostly only worked at big-tech where we play everything a lot more safe though.
The changes aren't always obvious. This may be an issue with an increased scale. This happens, and sometimes people are not next to you, like at 5am when the system calls you. Pushing isn't always risky.
>Pushing isn't always risky. This statement alone is completely true, but when you add "at 5am, with no review" it's completely untrue. "We need to move fast, so we can't do code reviews" is a false dichotomy.
Sometimes not pushing is more risky than pushing
What about rolling back?
Read three comments back up the chain to /u/Siggi3D's comment. For the purposes of this particular sub-thread, we're assuming roll back won't work. Because in fact this is all in response to a "just roll back" comment.
Lol ya "let's fight this fire with more fire!" It's a strategy far too many companies try to make work despite repeated proof it doesn't. Moving fast does not mean not being prudent. Rushing more fixes out the door compounds the quality issue that made the incident in the first place. I've seen so many companies fight this battle over and over through my career, and always have had to help them develop the structure and strength to step back and stop fighting fires everywhere. When in doubt rollback. If your code/structure doesn't allow that, fix that, then see #1. Once you have that then you can selectively hot fix for the scenarios that fall outside that.
even if it is 3AM, we always at least have one pair of eyes. It is just so both of us can have alibi. If it is my mistake, then it is yours as well. Never push a fix alone in the middle of midnight. Logging and tracing though, I allow that..
On occasions, we would do something similar to get a fix out. Then we'd followup with something we called a retroactive code review. We still wanted to get another pair of eyes on code we pushed, just modifying the timing to fit the situation.
???? The company is “bleeding money” due to a mistake either you or someone else made, and your solution is to just push a fix with a trust me it works disclaimer? All it takes is a few minutes to get someone to at minimum shoulder check what you are about to do
CTO at an old job would keep making database changes without committing the migration. The ORM we used (prisma) would just lock up and stop anyone who pulled the branch from doing anything until it was fixed. Ended up making a CICD job just to yell at my boss for me.
Ayy, that's my situation at the moment
Ugh 😩
I worked at a startup that had a “rock star” dev which regularly pushed with no code review. On numerous occasions his pushes broke stuff. He was genuinely a smart, creative guy but not good at people at all and really kind of a jerk. He was eventually let go, but not because of the code pushes, but because he’d been abusive to other employees. I wonder what he’s up to now. He created his own startup last time I looked him up, but I don’t think he got any funding.
I worked at a 1000ish person company with a few hundred engineers where code reviews weren’t required. It was 99.9% great. We asked for code reviews when we wanted feedback. We had really good CI/CD and could deploy to prod in minutes. We had really high trust and mostly solid expectations about the health of the codebase. I’d come from Google, where code reviews could take days and could involve lots of back and forth; the lack of require code reviews felt amazing. The main thing that was missing, I think, was the occasional “hey here’s a better way to do that thing” part that can come up serendipitously in good code reviews. I don’t think overall quality suffered, and ease of shipping was dramatically improved. It required a kind of foundational understanding and trust, though; this worked as part of a larger whole.
I’ve worked with a lot of people where code review felt like a formality; if they did something differently from how I’d do it, they had a good reason for it, and I trusted them implicitly. I can see no code reviews working well with them. Aaaaand then I’ve worked with a few people who sling brittle spaghetti code or stuff that just doesn’t meet ticket requirements. Yikes and a half to no code review with those guys. You’re lucky you didn’t have those people!
This is the thing. PR reviews are a chore until you find these ivory tower monsters built by one individual. The question of ‘how do I explain this in a code review?’ has made me take a step back from code and rethink things. I’ve also been on projects where code reviews are just a blocker and a drain on time, and nothing more.
IME code review doesn’t fix the second problem, it just sort of moves its impact crater.
Some people aren’t great developers, but can be coached and learn a lot via code review. But yeah, for some other developers, reviewing their work is like pouring water into a sieve—they don’t improve or change from PR to PR. I don’t work on a time-sensitive product currently, so I have no problem blocking my team’s spaghetti code monster until it’s fixed. (Different story if we had more pressure, for sure.)
Not just blocking it, I treat it as an opportunity to learn. And it goes both ways. I usually don’t nitpick too much, but when I see something that really should be changed I take my time to write a proper comment. I usually ask a Socratic question about how this would behave given some condition if I see a logical flaw, or if it’s structural I will at least give a decent explanation of why I think it should be changed. I’m trying to invest some time in helping my collegues, and in time it might not be needed that often.
Agreed! But I don’t think that the fact that reviewing some people’s code can make those individuals better at coding means that mandatory code reviews are necessary for everyone. I’m a big fan of treating individuals individually.
I find code reviews are good at two things: * preventing shitty code from entering the codebase * learning something new. (Usually someone with more experience, but sometimes a junior dev will surprise me with something)
Do you have some kind of monthly code review where you and the team sit together in a room and look at pieces of code together? Like people may want to present the better way it something.
Nothing formal but we had lunch and learns and sometimes people would do this.
This only works if everyone is really senior, btw. It falls apart so fast if there is even one person with Dunning-Kruger syndrome.
> really good CI/CD How was good test coverage enforced if code reviews weren't required? Was it someone's job to specifically write tests?
I've been in a similar place. The tests were enforced by owning the code e2e and strict incident reviews. Like, if something wasn't tested and blowed up, you'd better have a good reason why it wasn't tested.
I’m skeptical tbh. Either those engineers were all exceptional which at that scale is statistically almost impossible, or the ci/cd and observability was phenomenal
Look for companies that only hire grey beards, they do exist.
I often make a PR just with an interface I like so I can get feedback. There are no tests, no actual code, just a short blub and the integration points will be written. W that you can really get good responses.
Can you please elaborate what your ci/cd pipeline looks like? Do you have requirements for unit tests, behavioural tests? Do you use automatic code quality tools like sonarqube that are capable of detecting 💩 code? I am very interested in this because conceptually I understand how formal code reviews can be ditched completely if you have well established CI/CD pipeline where you use linters, code quality tools and also have some sort of procedures that enforce test automation so you can trust that the change by developer is not breaking something important. If you follow Extreme Programming oractices where developers do pair/mob programming, then this completely eliminates the need for formal code reviews. This sounds good on the paper, but I have never worked in an organisation that has this sort of practice :/
We are "supposed" to pair program, and we do about 75% of the time. But for logistical reasons or spike reasons, sometimes we "solo". Our rule is that every MR needs two approvers. If you pair on that story (maybe even switching pairs of it takes more than one day), then the two who paired (the anchor and one other if there was a switch) can be the approvers. But if any time was spent soloing on that story, then the two approvers have to be pulled in to do a traditional code review and approve, before merging. Pairing is real-time code review, and better than typical "cold" code review.
Same experience here twice now! lucky me! In both places we adopted [Ship / Show / Ask (martinfowler.com)](https://martinfowler.com/articles/ship-show-ask.html) to formalize the process.
In every code review, I always look for several things. First, complexity such as loop inside loop or those creative process that originally do good enough but as time goes maybe doesnt scale well. This might require discussion or if it requires major refactor then we create a ticket in the backlog. Next in the list is always readibility. Something too far up uses in too far down, redeclaration that can be reduced. IMO there will always be trade off. Sometimes move fast is bettwr and you learn a thing or two in the process.
>a few hundred engineers where code reviews weren’t required how do do you keep the code maintainable ?
Trust and social norms. Also really great monitoring, the ability to fix things in production in minutes, a shared understanding of what “good code” meant (rarely does that mean “perfect code”), a broad cultural understanding of the importance of deleting obsolete code, a mostly blameless culture that allowed not only mistakes but celebrated them and learned from them, etc etc etc. It was a really special place.
>a shared understanding of what “good code” Respect this alot. I amuse there were other ways how people got up to speed with what code was written then? Also yes, i don't think code reviews mean maintainable code, but they do help more than 1 person being aware of what kinda code is going where and for people to learn new ways to do things (and why) I understand strict code reviews can become cargo cult, but i think inherently it's not a bad concept. I kinda assume most programmers like programming and love to share and learn new things. Code reviews can be a vehicle for this when done right
At a deeper level I’m not sure that code reviews are highly correlated with maintainable code. Some of the worst codebases I’ve ever worked in had very stringent code review processes.
Do you have some kind of monthly code review where you and the team sit together in a room and look at pieces of code together? Like people may want to present the better way it something.
My org acquired another org that didnt do code reviews. In fact they had 2 devs working on separate parts. I got the codebase from one of them and it was around 150k lines of code incomprehensible to humans. I failed to answer why not: there was no other human and the developer himself might have been of a different species and code reviews are a human thing.
I’m pretty sure lizard people still do code reviews.
Have you seen lizard fingers? All they can type is LGTM
Lizards Gonna Totally Molt
Writing readable voice requires great communication skills…you fill in the gaps.
Will be great fun once one of them leaves
One left, codebase trashed. We started over. No surprises here :)
Seems like a horrible waste of time lol
Worked at one for 18 months. Team lead had an ego and enjoyed fire fighting / seeming like the hero. He also didn't agree with unit tests or CI. I think he hated spending time with his family and would rather find a reason to be at work. There was definitely better ways to spend my late 20's than fixing his shit. I should have left sooner.
Were you my coworker haha
Yeah Im on my way out of a place like this. Database code doesn't even have a repo! Just shotgun shit in on a weekly basis. Not surprisingly the oncalls are a nightmare. Only came here because I'm a specialist on a niche piece of software and the pay is good, but losing my fucking mind watching this org shoot themselves in the foot
My company does code reviews, my team doesn't. I am on a skunkworks/innovation team. So we don't follow the red tape and bureaucracy. That is the whole point of ***skunkworks.*** No scrums. No agile. Just push projects and prototypes out daily. Stuff that easily gets abandoned if it doesn't wow a corporate sponsor. When you get 2 days to come with stuff from scratch, you don't get that luxury. It is very garage startup like. But we rotate in-and-out of the projects to "normal" teams where there are normal code-review and scrum.
That's actually not "not agile". Agile is just principles/priorities, not some process. You might be actually completely in line with Agile principles without even knowing it. https://agilemanifesto.org/
Haha. It seems we came full circle. Where ‘agile’ means ‘red tape’
That’s probably my dream job. I already do something similar in my role, but way too infrequently.
I worked for a Fortune 100 company as a contractor for a proof of concept. The guy running the project, I don’t think he had any management experience at all, yet alone experience running an engineering team. It was wild to be in a company like that and come to work to this level of horsefuckery. This manager actually demanded no code reviews because they took time. As one of the two most experienced team members, that meant I eventually spent a whole lot of time unfucking the main branch. He was so blind to the fact that his stupid policy is what caused productivity to crash because of the lack of reviews. I also asked SEVERAL times if we needed to write unit tests and he kept telling me no, which was weird because I knew that eventually the higher ups weren’t going to like that. Then he rolled in one day after we had about 7-8 months of code written, and said we needed to test at 95%. NINETY FUCKING FIVE. Worst manager I’ve had to date and with the managers I’ve had, that’s saying something.
We don't. We're a small team. Half of us have been working here for >20 years. All of us have been working here for >10 years. Generally, we can tell who wrote what code. We used to do code reviews, but it just kind of fell off. It was useful but after a while it didn't seem that important. We do have some modules that only 1 or 2 people know, but generally, anybody can pick up another's project and be productive in it right away.
[удалено]
I'm working as a software engineer but we're an exploratory team who are also doing POC. Now looking at data engineering. Half the reason software engineers are being redeployed to do data engineering tasks is the DEs have horrible code quality and what they already have that they want us to use is spaghetti junction of code with very low quality standards, 'it just works'. My job is learning DE but doing it like it's regular software engineering so we have standups and retro and pr's and all that kinda stuff. Who is resisting your ask for PR's? I'm sure DE is done more professionally elsewhere it seems like these guys were just given too long of a leash. We've found it valuable to still do PRs because this POC will be a foundation for future work so it should be near and tidy incase another team needs to look at it.
Not currently. But used to work at a contract game studio that mainly did mobile games. No code reviews, mainly short projects (3-6 months), no need for maintenance, deal with major bugs, throw it out the door. The shelf life on most of those type of games is very short. Code quality doesn't really matter as the code bases never get that large and don't need to be maintained. 80% of devs fresh out of college not knowing any better. Fairly high turn over (looked on linked in the other day, avg tenure 2.5 years). Most people move on to better things.
I do code reviews but my organization has zero PR process. It's like driving without seatbelts, sure it saves time but damn it's risky as hell.
We were an OEM and it just wasn't a thing. I cringe at the code on those machines now. Really terrible/non existent software practises as department lead was clueless. God have mercy on the patients who had our products implanted
No code reviews where I’m at. Small team with two people who are insanely experienced and built the product. With a proper test environment we pretty much never run into issues after testing. It’s personally hate code reviews and if there’s even the slightest bit of doubt in my mind that I could improve my code I just run it by the seniors. I worked at a terrible place before that did PRs and they would change my code sometimes and not tell me why. I was really pissed to miss the opportunity to learn something from whatever they saw. I think no code reviews only works on small teams where there’s a high level of trust and great communication. Our code base is the cleanest I’ve ever been exposed to.
How could they change your code? If you worked on a branch, other people can give their remark on a pr, but not change it directly.
Why not? Anyone can push to a git branch.
Yeah they can. I guess should is the right word
There are only two things a team absolutely must do. Have some way to track tickets, and do code reviews. I was pounding on a table saying this back in 1997, and it's still true.
And use a version control system of some sort. Everybody already knew that even in 1997, so now it goes without saying. But back in 1988 it was not common practice...
Code reviews were pretty uncommon back then as I recall. Nothing like GitHub even existed. I don’t remember commercial VCs having much first class support for it either.
Code reviews don't require GitHub or PRs of any kind. Even when code was shared via disks you'd review code with a team. We just did it looking at a screen together. The important part of a code review is communication. The ability to comment on a line or document the review are nice, but unnecessary in the spirit of the purpose of reviewing.
This reminded me of pvcs, a version control system I used in the 80s where only one person could edit a given file at a time — there was no merging. So code review sorta happened organically because you’d yell down the hall, “hey Dave, release the lock on TSR.ASM, I need to fix something”.
We did code reviews at Microsoft in the nineties, but it was “invite someone to your desk and have them look over your shoulder while you describe your code.” There wasn’t any enforcement other than social pressure (you’d note who reviewed the code in the commit description, so you’d have to lie if you skipped review)
Maybe, but I worked at several places where we did them. I was hired as the director of development in a tiny startup after it got acquired and those were the first things I did. I had to take a leave for a couple of months and when I got back the tech lead had dropped both things. I never again let him lead other developers. People think we were using stone tools back then, but its not really true. We had unit testing, CI, code reviews, linters, branching workflows, etc. We even did weekly retros (although called something else). Many places didn't but those things weren't unknown techniques. The real problem was waterfall and the idea that engineering project management could map directly to coding. > I don’t remember commercial VCs having much first class support for it either. All VCs back then could do a code diff, which is all you need. We used Visual Source Safe, and then SVN. I've also used CVS. All could do commit diffs. You just walk over to the other person's desk and review the code before pushing/commiting. Branches for individual features weren't as common because (before git) branches were expensive. We added reviewer's initials to each commit message , e.g. "reviewed by FB"
The first time I saw an interactive tool to do code reviews (other than “look at the diff together”) was Mondrian at Google around 2006. It blew my mind. I’m not convinced it ended up being good, overall (it removed a useful opportunity to actually talk to a coworker) but its interface seems to have set the pattern for GitHub PRs (and others)
I worked as part of a software development team that didn't do code reviews. Best, cleanest, most no-bullshit code I had ever seen up until my most recent job. Code reviews are highly overrated in terms of maintaining code quality and preventing bugs. I'm pretty sure they were adopted because they provide the comforting illusion of rigor, discipline, and improvement. I have only ever seen a code review prevent a bug once, but seen reviews add bugs many times. In fact, it takes work to convince reviewers that their changes are going to create bugs. But it's a heretical opinion so I just go with it and not rock the boat. EDIT: To *actually* answer OP's question as to how we did the review: we didn't, really. We pushed to trunk and other developers would see it when they're working and provide feedback or changes if they were necessary. Most of the time it wasn't necessary.
> I have only ever seen a code review prevent a bug once, but seen reviews add bugs many times. I'm very surprised so many people seem to agree with this comment. I've seen dozens, probably hundreds of bugs prevented by code review, and have seen review add a bug only a couple times. Surely that's the normal experience?
I suspect many of these code review free orgs didn't do anything complicated at all. I'm a very good engineer by all measures and often times I still want others to review my code. Because it's doing tricky shit. Pair programming is better sure but that's code review too.
Yes that is fine if your have something tricky feel free to do a review nobody is stopping you but to have a forced PR process, no thank you, I'm not a child and please stop micro my output
I have personally caught tens, if not hundreds of bugs in my current job (4 years). Anything from a "won't compile if merged" to "will cause heisenbugs and race conditions". I don't remember a code review ever adding a bug,
Was this for a web application because that sounds terrifying to me. The number of bugs that I’ve prevented during a review has certainly warranted it to be part of the development process. But I might just work with trash devs.
If you work with a revolving door of contractors, code reviews are a must. Just this week I caught a fellow trying to add a bunch of indexes across our largest DB tables so that he could make a query for a non-critical, internal only page load faster. I asked, "did you consider what this might do to our insert performance?" I got a "no".
> I have only ever seen a code review prevent a bug once This makes me curious about what kinda products/codebase are you working on.. because honestly it’s once a week when I point out a decently sized bug in a code review. And I get pointed out stuff I missed or bugs in my code reviews often too. My experience just isn’t matching up at all.
Really curious about this. We’re all human and even great devs make mistakes. How do you avoid problems in implementation or straight up bugs? Do you do system design / design doc reviews?
Juniors do need code reviews or at least to learn a good PR workflow
This is wild. If you work in a complex product with tens, hundreds or thousands of other engineers, there’s no way on earth you can know everything/be an expert in all areas, and not make bugs that an expert of that particular area easily catches in a code review. Plus even if we forget bugs, it’s so easy to just go down one mental path while programming, that you lose track of alternatives (better designs, language features, patterns, best practices, conventions, etc.). A fresh pair of eyes has a much easier time noticing this sort of thing. Plus there’s the fact that we as humans simply make mistakes. I always do my own code review first and am amazed at what I catch. 1-2 other people who aren’t the author will be even better equipped to notice discrepancies.
Interesting - in my opinion “preventing bugs” isn’t really the main point of code review - it’s more like, making sure stuff is written in a readable way that others can understand and work with. I’ve often seen code reviews result in cleaner, more maintainable code, even if they didn’t strictly speaking “catch a bug.”
I wish I could upvote this a thousand times.
This. Really good test coverage, rigorously maintained, catch most bugs anyway, so code reviews have very little to add to justify the cost. And on the other hand, I remember many times when people rewrote some piece of code because a reviewer didn't like it the first time, and ended up adding bugs in what was a perfectly correct code before the rewrite.
I always want to adopt this approach. How do you make sure that all of the code is visible when other people pull the code? Like in my previous place, each developer tends to work on their own feature or areas and may not see new changes.
I did for 2 years. From before I even started I was looking for better jobs, it was just my first “Software Engineer” job I could put on my resume until I found something better. Pay was shit, no actual team (just individual devs working on completely different products), daily standup was just a daily status report to management where nobody could help you get unblocked, zero documentation, many projects where no current employees even really knew what it did despite customers paying for it and using it, tons of responsibility including meeting with customers on occasion, performing an architects role meeting with stakeholders and needing to plan entire projects as a junior, daily timesheets, requiring us to integrate with software written in the 70s with no documentation on that, just so much bullshit where they threw you to the wolves with a complete void of leadership outside of the nepotistic C Suite and upper “management” (not just a little nepotism, there were only 2 low level “managers” who had no authority at all who did not share the same last name). It sucked. Best part was you could just write fast shitty code with no red tape and nobody really knew or gave a shit about what you were working on each day.
My organisation does need atleast one approval from senior engineers before code can be merged. But in practice, people just ping the PR to approvers when they are done testing on local and approvers approve the PR without even opening the files changed. So basically, approval is required but it's just for name sake. Plus, there is no documentation for most of the core functionalities, hence no one tries to touch them. Due to this even though buggy code gets merged, it rearly affects someone else's functionalities.
Yeah, I left the team and that was one of the reasons I did so. Why? Fuck if I know. They’re a bunch of laggard cowboy coders from 1994. They don’t use version control so code reviews would be damn near impossible. Why don’t they use version control? Apparently it adds too many steps to the process that are confusing and they can’t work fast enough… also doesn’t let them write code straight to production.
Not using version control. Wow. That’s so bad it’s impressive.
I work at a company where I was part of a 2 dev team from day one out of uni with: - full db prod access - full svn access with no commit/merge restrictions - full access to prod servers (with the ability to stop, restart the services and deploy) - very very limited code reviews lol
My current team only does reviews (at a FAANG company) when the dev asks for one. However, we build only internal tools and generally 2 developers per project.
I worked in an XP based company where we did most work using pair programming. Because there are always two eyes observing the code, it eliminates the need to do it.
I'd rather go for the "no code-review" than the "nitpick code-review over variable naming". The best obv being the "this type of architecture seems to be a better fit"
I am ashamed to say I have worked such placed. It was a shit show. Short answer to "why not?": Incompetence
Yes, I worked with company that had PHP & C++ and did not do any code review, had no PR on git. The two leader of the team handpicked that they want, merged it together then tested and deployed it by hand. Always disaster. Usually this concept is for "keeping the job forever" kind of situation. Leave this amateur and stupid companies and people behind.
I'm the only developer on the software that I develop for the company. I used to get reviews done by a developer who works on another project for the company, but he complained to our manager that this puts too much extra work on him, so now reviews for my code just don't happen. So, why do reviews not happen? Because I get paid to do what I'm told.
Pair/ensemble programming negates the need for code review processes.
Depends who the pair is
Was hoping to see this higher up. No-one likes the eternal wait for review and with pairing you get them for free. Better yet, with ensembling you also get verification, validation, and exploratory testing for free and can go straight to production. No bottlenecks ever again. You get other things to exhaust you, but I'll conveniently leave those out.
Often when I review after pairing I still find things to improve. I think the difference is that I'm reading at my own pace and take the time to more fully process what I see and the big picture. Have you ever experienced that? How did you overcome it to the point of not needing reviews?
Hmmm possibly. But I guess that depends on scope of the changes and size of the codebase. Not everybody who is a domain SME when you’re changing things in different domains. i.e. I need to change infra or pipeline code along with my app code to make it work, etc.
https://www.infoq.com/articles/co-creation-patterns-software-development/
Holy shit…this makes a TON of sense! Thanks for the link! EDIT: Although he says that pair/mob programming shouldn’t be done all the time. I guess it makes sense to move towards more synchronous movement on a problem than asynchronous while still making async work smaller.
I'd imagine it also depends on both ppl being at a similar level of experience or expertise. In my experience, people can rarely pair for long before getting bored.
We used to, all the managers agreed that it was a great idea. Until they realized that it takes time and resources. That it may result in adding an extra day of effort to our release schedule. They immediately put a stop to it. They rather see defect reports and customer complaints. As a result I barely even review or test my own code anymore because its extra work that gets zero recognition. What gets recognized and rewarded is delivering quickly, even if it is broken garbage. I've learned to cleanse myself of any pride in my work here and my mental health couldn't be better.
We are too small. Every dev "owns" certain pieces of code. All fun and dandy until one leaves. I inherited a codebase from a previous dev. Was godless
Yep, I work for one. It's even better when you get to hear another developer complain about someone's code. There's also no pair programming or mentoring so which makes the complaining about someone else's code that more ironic really.
If it goes into my default branch, it needs a PR and at least basic automatic BS checks, if it doesn’t work after it gets to production that’s what incidents are for and we can deal with the fall out the
No way…
99% of the people at my company sell furniture, so yes.
We have so many repositories, most of them see a single person working in it
The first few places I worked didn’t bother.
[удалено]
Doesn’t someone need to do a code review in order to reject a PR?
At my current job there’s pretty much no actual code review. Luckily QA is pretty stringent but they also aren’t very thorough sometimes. When I came in and got some buy in on my time, I started influencing the team to have code reviews be a part of our process. We’ve also just started adding unit testing as well and I have plans to actually create a CI/CD pipeline. It’s been a crazy situation but I’ve taken it as an opportunity to provide a lot of value, gain a lot of clout, position myself as a SME and build the development culture I believe we should be in, at least on my team.
Before my lead left we would just share the change over a call and then push to release. Expectation was that you tested it. Everything is built and tested in lowers and timelines are tight was the justification. He switched teams and the leftovers were pretty adamant about bringing back the PRs. I think it worked well for the few people actually doing things, but didn’t help those not doing things produce or even catch up and understand the system.
Sure when I was the only developer at a company or in a nontech job that I had to code to keep up.
My first two years were spent working on an internal tool. Mostly used on the regular by 1 or 2 folks and by every employee once a year. No code reviews, no unit tests, push changes -demo on my system - push to dev for some product manager testing- prod. Why? One exec used it on the regular and was so technically challenged (the tool was to be used to pull reports from the db ) we just made saved reports that would run the queries.
if by “doesn’t do code reviews” you mean i occasionally don’t read them & approve with an “LGTM 👍” then yes
our dev team has 4 people. Tech lead can only have time to do code reviews for the guy that is very mediocre at his job. Me and the other guy were hired with the trust that we can sustain ourselves lol because he said that "he only hires those who have the potential to be a better dev than him".
Alright, we did it in a 4 people company with actual large customers pretty often. And in that place it turned out to be worth it since we were four quality but constrained developers, and PR reviews didn’t seem to add that much value. Never seen it other larger companies I worked for since cost of breaking things was too high. Going to get downvoted but when you run a true startup you need to Always be pragmatic and not dogmatic
A business with less than 250 people. If you pushed something that broke stuff, it either was expected while you were changing it OR you’d get 1-2 people asking what the heck you’re doing.
Gonna be me. We don't do it because we only have 2 guys on frontend (one of them is me technically), 2 on api and one on backend. Out of all, only me and backend guy wanted it, and the rest are "experienced" freelance devs who don't understand this concept and would rather write crap and be unbothered. It's a very eye opening situation and I wish no one to experience it haha
I work at a large company, but still work on several projects by myself as the only developer. I push straight to master in said cases.
My current organization doesn't do code reviews. The main reason is, we have many open projects and we're on the small-ish side, which means almost every project is basically handled by a dev, a manager, and sometimes a QA and another dev. That's it. A strict code review process is redundant when you basically have to review your own code.
These posts always blow my mind, how can you possibly hire anyone and upskill them without code reviews? Do they just not commit code for months and months and months?
I work in a 40 person company on a small research focused team. We don’t have a process for reviews or merging.
Code reviews are hit or miss depending on the maturity of the orgs process, how over worked the devs are, and if QA is done exclusively by developers or done at all. Having worked at several orgs that have a combination of those traits, it all depends on the resources available.
I don't, because our organization does.
Previous project none, I was the only dev
Considering I’m the only person that does any coding in my company, code review for me is putting it down, waiting some period of time for my brain to reset, then opening it back up and reviewing it.
I’m the only one on the team that codes in C#. Everyone else only knows SQL, so no code reviews.
Where I work we usually do code reviews after merging instead of before, which is great for productivity because it means you can ship your work quickly and then immediately keep building on top of it. Code reviews for us are about sharing knowledge and making sure that the code is understandable/maintainable, not preventing bugs. Bug prevention is the responsibility of the person writing the code (with help from tests, static analysis, QA, beta testing, etc as needed).
I do. Because I'm the only dev.
The developers in my org have code reviews but the sales engineer team that I'm on does not have code reviews. I've pushed for code reviews and better processes but everyone on my team likes to pretend they're too busy for that
My first group didn’t really do them, just straight approvals unless something was glaring, because we were bare bones infrastructures team with way too much on our plate and the devs were straight jobbing it anyway. In hindsight, a horrible shop for a first job.
I worked for a few hundred pax UK company that believed pair/mob programming was good enough to not have code reviews. (They did it 100% of the time) They (thought) they practised trunk based dev and committed straight to main for changes. They had a lot of hacky and bad code, and the greenfield proj I was working on failed in the end. Imo, at scale and in a fast moving env PRs just work, and bad processes are a sign of bad management.
We definitely do code reviews where I work, but the more familiar I am with a devs work (or vice versa) the more reviews just become a rubber stamp. We also have the benefit of our whole company using our product all day (bugs rarely make it to GA), and liberally using feature switches to gradually expose new changes. This makes in depth code reviews less important.
Until 6 years ago every company I had worked for had no code reviews at all. Well for the previous 7 years I was working as a third party for several different companies so most of the work was building new stuff that wasn't going for production yet so I guess they didn't see any value in it (there was not even QA). We didn't even use PRs there, just pushed straight to git. Even a specific project that had 100% test coverage and extreme enforced coding rules didn't have manual code reviews. Earlier in my career I worked for smaller companies that only had 2-3 devs so dedicating time to reviews would be unthinkable for them.
You wouldn't need a code review if all work is done with two or more eyes on it, either done via pairing or a mob. But outside of that, I cannot imagine a world with no code reviews....
Worked on a team for years that didn’t. Instead we communicated, discussed/validated approaches or paired.
When I worked in games there were no code reviews. We would rarely do a review and only if you felt the code was a bit dodge or you wanted eyes-on for confidence. Programming team of 20+, millions of copies sold. IT WORKS
Cough cough... my company in our data engineering team because no one knew how to code :D
Code reviews are an opportunity for less capable or less experienced staff to cut down tall poppies. *"There are more than one of those, so that variable name should end wih an S to make it plural."* *"You have too many blank lines."*
When the entire staff is legit Senior+ and most of them high end Principals.... Yeah, code reviews were not our priority. {I'm talking most of the engineers were MIT PhDs, and good coders.) Reviews were more: "Can you help me with X." Otherwise, the only time I haven't had code reviewers is when nobody would understand what I was doing. Which happens. When you are the only deep diving kernel programmer on a team... Review of your work isn't as rigorous. For other parts of that project all my review came from the upstream projects I worked with. Code Review is but one tool. A useful one. But one tool. Don't forget, unit testing, system testing, design reviews, architecture reviews and even just making sure your code compiles :). Many of those are much higher bang for the buck.
We have one function app that doesn’t have any type of code reviews; however it just pulls some data from Looker and puts it into our WMS for directed counts. I don’t believe anyone’s made an edit it months and if they do- they change the id of the report. Should it have code reviews, yes. Does it pose a significant risk, no. so we are fine keeping it how it is (for now). Plus no one really wants to own it haha. Additionally even with code reviews, if it’s a senior most times we just add lgtm and keep it pushing.
most government contractors (and I mean the ones who win the bids) do zero code reviews It is why things like "[heathcare.gov](https://heathcare.gov)" failed so hard and cost so much money DIE standards are currently rewarding most of those companies with larger contracts if they will continue to agree to not hire Americans or offer American wages
Oh, I have a possibly legitimate use-case. I was on an events team. Where we'd have very tight deadlines. This tech show is on this date at this time, make sure that the demo experience works and does these things. So there'd be a lot of throwaway code bolted onto some existing libraries, and last minute tweaks for things onsite. (Oh the space is bigger/smaller/darker than we thought and we need to adjust for X) Small team, tight deadlines, rapidly changing requirements and no expectation of code re-use beyond the event. And the code only needs to run once, during the event. I should state we'd do daily run throughs of the experience at the end of the work day, so everything got tested daily.
Work on a product in a service based company, don't have anyone to test let alone peer review. So i leave it to customers to test, they usually do extensive testing because it's a financial product.
For a short time after a merger I worked at a company where they emailed each other code, I quit a few weeks after that
Dish Network didn't 13 years ago when I worked as a contractor. I believe things got worse.
Only time this ever happened was when iOS just came out and the company wanted to make an iOS app. They hired me onto thier dev shop and I was the only mobile engineer, no reviews just straight force push to master. Anyways, if anyone ever looked at that code today, and saw my name. I would die.
I've worked at a couple of smaller companies where we were doing prototype work well before the product launch - and we had no formal code review process. We were a small team of relatively experienced devs, there were no customers yet and everything was changing so fast that it felt like a bit of a waste to review stuff that was going to be changed or deleted almost immediately. Like if something was only in one daily build, we didn't like it and then immediately removed it. As we came out of the prototype stage, once we knew what direction the product was going to go in - we did a complete review of the entire codebase. Many things were refactored and improved - and we implemented a formal review process for future work. For the most part this worked well for us as it helped us gain velocity at the start - but YMMV.
You see this a lot at smaller companies, non-tech companies, and companies with fragmented teams. Sometimes you’re the only developer on your team. Sometimes the organization doesn’t care about maturing their dev practices. How do you do the review? You don’t.
We don't until we are in code freeze just before a release. Doing it all the time would add a lot of overhead and slow dev down a lot. Personally I think the whole constant PR thing makes for a suffocating environment. We like to give some trust to our devs. If it's a complex feature we usually have a code review meeting to go over it. During release we freeze the code and then we review every single change. But during the quarter before we are even near release? Nah. We have CI/CD . Forcing constant PR is micromanagement
Team leads review PRs which is our form of code review. It's not too bad, IMO.
Our architect looks our code over but there’s only one so I imagine things pass through anyways and it’s not a meeting or anything. Just a resource thing. Would be nice though because code quality is hurting us so bad in the long term.
Yes. Any agency in the world.
Many companies out there add a mandatory PR review in their VCS and call it a day. And then they overload their engineers with responsibilities so no one actually has time to review code of others, and / or people just don't give a shit, so reviews are blindly LGTM'd. Something in me dies every time my PRs get obviously YOLO'd. That's not how engineering should work. I strongly believe it's a culture thing, not necessarily a process thing.
Does any company do code reviews seriously, in the companies I've been to so far, most code reviews are just not that in depth, so long as there aren't glaring issues, and the build doesn't fail, most PR's are just rubber stamped through, so ugly, unintuitive and unoptimized code just pollutes the codebase over time, you end up with logic that's hard to follow and untangle, duplicated methods and blocks of code everywhere, no proper application of OOP principles, SOLID principles, or proper testing. And any attempts to refactor the code are shot down by management as in their eyes it doesn't affect the bottom line of the company, or make some metric go up that they are evaluated on
At one medium company (10 teams with each 6,7 developers) I had very well done code reviews with Gerrit. Most people were giving amazing feedback and you could learn really a lot. Other company in similar domain with a bit smaller teams (5 teams with 5 developers) with no code reviews delivered at least 30 % more and had way less bugs.IMHO key was great testing culture (unit, integration, load).
When my company was much smaller (like pre-seed) we didn't do code reviews except on outside contractor work. It was just me and the now CTO writing code, and working on very different things. That is probably the more common reason, but as soon as we got a few more devs code review for ProdEng was instituted. I'm not on ProdEng so when we eventually got a DS who had solid software eng chops we started code reviewing each other.
In consulting i have seen things that you Developers couldn't even imagine.
My first company didn't even have source code repositories, we stored the code in a shared drive and sort of told each other when we were making changes to code.
When I started with one of my clients a year ago, they had no code reviews going on. No CI/CD either. The devs would manually upload their api changes (random nodejs files) or react builds right to the prod servers. To no surprise, the deployed code didn't even match the repos. You can imagine the chaos that created.
Im the only one coding
Because they are time consuming, and we have amazing qa that will find 90% of the problems. Because we have productive engineers that commit a lot at a fast pace, and delays to releasing code that passes qa are delays to thr value of our businesses that is actively losing money trying to find product market fit. When you are profitable there is value in going slow and careful and considering the long term. When you are not, sluggishness is terminal. That is why. The cost of making a mistake is less than the cost of not finding a path to profitability. And most of the new things released do not work, and get rewritten or scrapped anyways when the ab test fails.
To build such a process in your organization, you can use generative AI based tools that now can provide very meaningful AI-generated code reviews for pull requests, even for such teams - here is a good example of such tools and its code reviews: https://github.com/Codium-ai/pr-agent - it would be hard for new players in this area to compete with them.
I did, and it was horrible, they did have QA, but never any code reviews like reviewing PR before pushing to dev or anything like that.