271–280 of 315 posts
Earlier quoted context omitted.
> Is that why we do code reviews, because we don't trust each other? I don't see any other reason to require code reviews before a change can be merged. > humans make mistakes Then write unit tests, religiously. And pair. Or "... request reviews when they think it's necessary". In my experience code reviews are not very good at catching mistakes. > Different work in progress can be in conflict with each other. Code r…
> I don't see any other reason to require code reviews before a change can be merged. In the US, a SOC 2 audit of your org’s change management process is going to be a really bad time without this.
He mentions "trust" a lot. Is that why we do code reviews, because we don't trust each other? I find this attitude problematic. We do code reviews because humans make mistakes. Requirements can be misinterpreted. Different work in progress can be in conflict with each other. Reviews are a good way to learn from each other and keep abreast of what work is occurring outside your own bubble. Not doing code reviews becau…
I never work directly on master, always on a branch, and when I'm done I create a PR for myself.
Can't remember a single PR that I reviewed where I didn't find some issue or bug.
I guess you can say I don't trust myself...
Earlier quoted context omitted.
> I want my code peer reviewed TFA: > Engineers [..] request reviews when they think it's necessary. Problemo solved.
If code reviews aren't an expected part of the development process and there's pressure on delivering, it won't happen.
It does however mean you don't sit around waiting for someone to approve a three line delete PR that you know is safe. The "Rubber Stamp" review is a thing and it does waste time.
> Pull requests don't prevent bugs. I remember once I was giving a talk on code review and I said that code review doesn't prevent bugs and people were shaking their heads. Everyone hangs on to this one thing, but if you look at the majority of pull requests that have been approved you'll see a lack of comments about pontential bugs. Also, if you do see a comment about bugs they'll often be disregarded. This drives m…
> I remember once I was giving a talk on code review and I said that code review doesn't prevent bugs and people were shaking their heads. Everyone hangs on to this one thing, but if you look at the majority of pull requests that have been approved you'll see a lack of comments about pontential bugs. that's because the UX for github PRs sucks. Use Gerrit instead. It's not as pretty but it's super productive. > I thin…
That's actually one of the costs of PRs (or any kind of mandatory and blocking code review). I've worked in places where the code review wasn't hard. People fixed these nits as a final step before pushing the whole thing to master. With blocking code reviews people either do that 'nit' thing, or keep the remark to themselves because it isn't worth fuzzing over.
All code review processes have some degree of rubber stamping, but the mandatory ones have a bit more.
Orthogonally, I feel that more reviewers makes the process and the results better. The best review process I've seen was slow as hell: (nearly) every team member would do (nearly) every code review. People would start riffing off of each other and actually learned from each other. You could actually get a good team-wide conversation going about the best approach to doing stuff a certain way.
Earlier quoted context omitted.
> I just don’t think this would work for most teams and developers. Most teams and developers don't do code reviews because the only person that could do code reviews would be the developer himself.
Do you mean to say that most developers work alone, so don’t have anybody to review their code? That’s probably true for hobby projects and the like, but companies rarely have teams of one working on anything.
Yes and no. They do work in teams, but this teams are so small, that everybody has 'their' part of the code, where nobody else even takes a look at, much less changes anything. I'm talking mostly about non-software companies.
Btw. most people that actually do work alone are self-employed. But code reviews don't make a sense if there is no other developer.
Earlier quoted context omitted.
Sure. But if you read my previous message this relies on people being comfortable asking. I am but not of my colleagues are. The formal process would let them use the benefit that I can use when I think it’s necessary.
> people being comfortable asking. I am but not [all] of my colleagues are. Then that's what you need to fix.
Earlier quoted context omitted.
If code reviews aren't an expected part of the development process and there's pressure on delivering, it won't happen.
I don't believe that to be true. I believe the expected outcome is that people will still want code reviews on anything vaguely complex as insurance against fuckups . "Shit, WE missed something" is a nicer place to be than "I was sure I was perfect and I'm actually an idiot". It does however mean you don't sit around waiting for someone to approve a three line delete PR that you know is safe. The "Rubber Stamp" revie…
I'd be exactly in the boat you describe (and actually: I've yet to see a situation where someone reviewing my code did not lead to improvements). But: If I want my code reviewed, I have to actually fight for it. Since everybody's workload is too high, even the willing often simply don't have the time.
From higher up, at our place, there's no one that actively opposes code review as a practice, but they don't seem to get that it requires investment (i.e. time), either. That means that if management refuses to change their ways of planning or explicitly enshrine reviews as priority tasks, it simply won't happen except for maybe the gnarliest cases.
And that's from a perspective of the change's author being willing and proactive. I work with individuals that produce very questionable code at times. And I'm not talking style here: Code in 2021 that still is prone to SQL injection at every turn. Methods that are hundreds of lines long with a gigantic cyclomatic complexity that no one (including the author) will ever really understand. Method names like "process" and "process2". And so on.
Needless to say, those programmers will never push for their changes to be reviewed. The rest of the team simply discovers them after they blow up (which they do regularly) and we're debugging.
Now, these people are luckily a tiny minority (which at least makes this somewhat bearable), but I over and over again would wish for someone with more authority to show that they're interested in the matter. Which a policy on code review would do -- if only as a signal. Yes, we want to check each other's code. Yes, we want collaboration. Yes, that's more important than some arbitrary deadline for minor feature X. Yes, we expect our programmers to be professionals.
Another symptom of such an environment also is the fact that everyone is the king of their particular hill. No one every familiarizes themselves with another person's code unless they really have to. By now, I've jumped to insisting that every change on a critical component I'll ever make will be done through pair programming, and so far, at least that seemed to stick. But again -- that only happens if both parties involved actively fight for it instead of everybody understanding that this is just an expected part of work.
He mentions "trust" a lot. Is that why we do code reviews, because we don't trust each other? I find this attitude problematic. We do code reviews because humans make mistakes. Requirements can be misinterpreted. Different work in progress can be in conflict with each other. Reviews are a good way to learn from each other and keep abreast of what work is occurring outside your own bubble. Not doing code reviews becau…
Yeah, IMO this is like a publisher saying "we don't have editors: we trust our authors". The intended ethos of editors and code review is that mistakes and imperfections are the norm, and you need a second pair of eyes to iron them out. That said, I do sometimes encounter individuals or cultures that seem to view code review more as a mechanism for catching abnormal/unexpected mistakes than as a normal part of the pr…
No it's not.
If an author produces gibberish and nobody checks, the published result is gibberish. And with print books uneditable in a run starting in the thousands.
If a programmer produces gibberish, it won't compile.
It also won't pass the unit test or acceptance tests, never mind QA or the alpha/beta testers.