I was really hoping for more advice on letting the team members appreciate code reviews, and not take it too personally.<p>We've just recently introduced code reviews on our 3-4 person team. We're working in a particular environment, and most people have no production level experience in that environment. These people have gotten used to _just getting it to work_.
We've clearly stated that we will be going with Google's style guide, but we can always divert, as long as everyone is in agreement. When pointing out that the changes are not according to the style agreed (frankly, it's not so much about style, as it is about using patterns from other programming languages, that are not applicable), the response is:<p>- it works<p>- I don't think you understood what this does (note: no comments or attempt to explain)<p>- In the future, please do not waste more time with pull requests. Desktop sharing and a call is the most efficient way to work in remote teams.<p>It's getting more and more hard to keep an open mind and not take things personally. We're at a point now, where there's a intermediary person refactoring the code.<p>Does anyone have any practical advice on people understand the importance of structure code, and to limit the _personal factor_?
I've experienced some really toxic cultures around code reviews. I believe the bottom line is teams need to set standards; linters and findbugs should be incoporated in the build and fail the build if formatting standards aren't met. Standards for level of abstraction should at least be discussed. Is using exceptions for control flow ever allowed? Requirements should not be defined in code reviews. I go so far to say that code beautification comments should be handled in separate tasks.<p>Above all code improvement requirements and comments should be based on industry defined best practices, like citing SOLID principals for example. Or, citing agreed upon team standard for appropriate level of abstraction. In short, there has to be a limit to the pissing matches.<p>I spent six months in a Big Dumb corp that did none of the above. Every team member reviewed everything and every comment had equal weight. Code review participation was part of the performance review process. Sounded nice in theory, but incentived a truly toxic environment of "my solution is more clever" and "my solution is better use of abstraction". Which resulted in a spaghetti mess of over engineered code that still had bugs, and still didn't meet requirements, even after 100's of hour of reviews.
These experiences mirror my own in an uncanny way (recently made team lead, 3-4 devs, just started code reviews) and found all of this information extremely useful. I would add that discussing how much mileage can be estimated on the current stack is important as well. Good code can still be dangerous if someone in the team knows or feels they are only going to get 3 months out of it before dependencies will change and will need to update it again. I've found discussing sustainability from time to time helps to figure out who is checking in their code just to make it to the next day and who has an eye towards creating a low maintenance product. Life pressures can cause the best devs to live one day at a time they just need the proper forum to express it to the whole team. Sometimes it just simply gets forgotten or lost in the shuffle- the fact that library or API "X" will no longer be available.
I've been code reviewing everything for my entire team for about 6 weeks.
The idea being that we have some kind of quality control on the stuff being developed.
We're using TFS to ask for reviews, which actually works really well.
...what doesn't work is the junk I keep being sent....I bounced the same review 3 times yesterday because it didn't even compile...
When the stuff you get is decent reviews are ok....when it's junk, it's soul destroying
>when it was time to review code, what we really looked out for were obvious bugs, problems with the design or architecture and where code should be located. Variable naming was inconsistent but no one really bothered to enforce them. I don’t blame them, making code review a nit-picky task really puts a damper on the mood.<p>As a veteran of a dozen different code review approaches I've identified a few properties of those that succeed and those that fail.<p>If you want your code review process to fail its easy just...<p>1. Make it about you and your preferences. Does this code adhere to my particular aesthetic preferences? Are the variables named the way I would name them? Do I consider this code readable? How can I force others to adopt my perspective? Remember the purpose of code reviews it to keep iron-fisted control of the code base such that it never becomes that dreaded "big ball of mud"<p>2. Demand that the team adhere to a set of rigid rules regardless of how practical their application is.<p>3. Most important focus mainly on the subjective qualities of the code(format, spacing, naming, etc.) Allow non-critical path items to hold up delivery and use those items to critique others based on an arbitrary measuring stick.<p>If you want your code review process to succeed. Its a little harder....<p>1. Be problem focused. What has bitten you in the past? Are you sure you understand the cause? Acknowledge that there is no "right way" and that you will build the perfect code review model through trial and error.<p>2. Start with the bare minimum and build on it with a regular retrospective process. Allow your team(s) to take ownership of the code review process both as an expression of what they want to accomplish and as a way to improve their daily lives.<p>3. Accept that you might have been the one doing it wrong all this time.<p>4. Focus on the objective qualities in the code. Is this the best approach to solving this problem? Does it work? Are there any obvious bugs? Based on our collective experience will this code cause problems later?<p>5. If you feel like you are nit picking then you are nit picking. Don't nit pick.<p>Code reviews can either be a tool to write better code or a form of weaponized OCD. Don't be the latter OP, you're better than that.
What do you all think about the practice of pushing changes to an open pull request based on code review feedback? My org has not established a consistent policy around this and often people will push changes after some reviewers have already approved the request, which I find concerning. On the other hand, it is nice to see review comments being addressed in the same branch/PR. Thoughts?
Good start and my best wishes. We follow these on stash using fork & pull request model, for a large project with 10 teams, each around 6-8 devs.
- everyone has a fork
- no one commits outside their fork
- each module has 3-4 reviewers
- good practices are constantly shared via sessions
- wiki drafted and updated
- comments are given reasoning with concepts, discussion threads and as well to other modules
- object calisthenics, if required