181–190 of 315 posts
If you have <10 is it an efficient way to work? Yes, it can be. Replace the formal process of code reviews with an informal process of reviewing new commits. Encourage everyone to regularly read the code and not silo themselves away.
Earlier quoted context omitted.
The best code I have seen was on projects without code review. And projects with it were more mess - they were surface consistent but overall hard to comprehend. The deciding factor was ownership and accountability tho - you maintained own code and if you done it crappily, you knew. The code review is related to assumption that everyone can change everything - meaning all in all inconsistent mess. It is also related…
I prefer a top down where project leads are responsible for code, I.e. they review all code and they are simultaneously responsible for getting things done. It forces balance. I will let juniors make a mess in certain areas and not others. Sometimes you even let bugs in if you know a specific test will fail so people are more careful and don't depend on reviews too much. If you are a bad lead and nitpick every little…
You certainly don't want to work in a codebase riddled with different ways of doing the same thing. Code reviews help with making the patterns more structured and more widely adopted. It also helps everyone at all skill levels to learn something from their peers, through reading their code and sharing thoughts.
It also makes more people in the team familiar with the codebase, such that more people are capable of changing and improving the related parts rather than just the original author.
The potential benefit of finding defects is only a nice to have!
Earlier quoted context omitted.
> Code reviews are not a tool intended mainly to catch bugs Well, not to detract from your point (I agree), but what little research has been done in this area suggests that code reviews are actually one of the best ways to catch bugs: https://kevin.burke.dev/kevin/the-best-ways-to-find-bugs-in-...
The source actually says that informal code reviews are very poor way to catch bugs, while formal code reviews are one of the best. I don't have "Code Complete" at hand to follow up on that, but I strongly suspect that the formal code review was defined in this research as reviewing the entire codebase line-by-line on meetings. As opposed to reviewing just a single change to the system. Can you/anyone shed some light…
Earlier quoted context omitted.
>Code reviews are a huge time sink, especially for unimportant style- and naming nits. This isn't a problem with code reviews, this is a problem with your team's processes that code review has highlighted. These problems will show up in others ways if not during code reviews since clearly there's strong disagreement on coding styles and conventions. So go and fix or implement the style guides, linters and so on. This…
> So go and fix or implement the style guides How do you make this converge across dev without endless debate or resentment ? > linters and so on. What happens when a rule has to change ? update the entire codebase impacting everyone with conflicts ? tolerate divergence existing code ? Talking about thermometers, are you actually tracking fevers, or merely minor bad breath ?
How I did it as part of a ~35 person dev team was adopt one of the popular ones from the community (e.g. Google's) - treat it as a benevolent dictator. Apply it to the entire code base in one giant shot (disrupting any pull requests that were in flight but fixing them proactively), then enforce it with tooling. Leave the door open to anyone who disagrees with the defaults. It's on them to propose the change with rationale then put it to a quick majority vote with a subset of the team. I think 2 things have actually changed since the very beginning (line length from 80 -> 120 and something else that I don't remember right now).
When something changes, making a sweeping change is part of accepting it - if it's too risky/disruptive then it has to be very valuable to do.
Earlier quoted context omitted.
Why would it be absurd? If there is a process to reduce risks, then people take more risks. I have been guilty of submitting code review when I'm not 100% sure it's perfect, just because I know that there is a code review process. So this definitely exists, not sure how common it is though.
Also, the trick to code reviews is to leave in a few low-hanging obvious bike sheds. The reviewers will tell you what color to paint them. Done. I'm being slightly sarcastic. But not sarcastic to the point that I haven't done just that. Just as the nail that sticks out gets hammered, the code that is too perfect gets increased scrutiny.
Earlier quoted context omitted.
For many developers, people merge their branches directly to production so the pull request review is where that checking happens.
Ok, but many companies don't use such ridiculous process.
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…
That final, 1 word sentence is one of the major problems of PRs. An engineer might spend several hours really thinking through a problem, talking with colleagues, whiteboarding options and coming up with a workable solution that addresses all the obvious issues and a bunch of non-obvious ones. Only for a drive-by take-down by someone with none of the context. It's a totally asymmetric investment of time. Even helpful…
[0] Not just because the reviewer is trying to be nice, but because they don't know what constraints and problems the author discovered in the process of writing the PR. If the reviewer writes "use a map here, instead of scanning an array" without knowing how many items are handled, then they may be making the PR worse by replacing a cheap linear scan over a maximum of 50 items with a bunch of costly yet constant time hashing. Often the solution is an explanatory comment rather than the "obvious" fix.
He basically described the classic subversion workflow using git. I feel like people rediscover subpar approaches using technologies which intended to evolve those approaches into a better workflow
Somewhere between those the context is such that the benefits of code reviews outweighs the costs. Do you think otherwise?
According to LinkedIn, this company has 12 employees. Once they hit like 20 - 50 engineers and some decide to write code "their way" and some leave, it's going to be a mess to be onboarded there and make sense of code you haven't written yourself.
- All senior developers who had a strong sense of the actual product and what they're trying to do -> no pull requests, everybody happy - An awkward middle area where eventually they had tripled in size and code quality was haemorrhaging customers and paralyzing attempts to release -> "what do we change?", nobody happy - Another doubling in size, giant organizational change -> PRs are mandatory, people aren't happy but any attempts to reduce PR burden has almost a straight line to bugs in production