5 ms·

I would add code reviews to that. The advent of those was when it really stopped being fun for me, because it's really hard to keep that from turning into "your

I would add code reviews to that. The advent of those was when it really stopped being fun for me, because it's really hard to keep that from turning into "your boss nitpicking every line you write."

oriolid 7y ago
In my opinion this is much, much better than trying to decipher what the heck that one co-worker pushed to production and how exactly it broke things. Of course there are ways to smuggle rubbish through code review, but it's still better than no reviews.
- randrews 7y ago
  Both have problems. The best system is one where a passing review means "I feel comfortable maintaining this;" too often a passing review means "this is how I would have written it."
  - Silhouette 7y ago
    IMHO, review processes should concentrate on identifying problems, not solutions.
  - wes-k 7y ago
    Agreed. Few things to consider: - linters to help move away from code style comments - Peer code reviews (if possible) - inform your team/boss of bike shedding - use a tool like reviewable.io where comments can be marked as blocking or not. I’ll often comment on preferences and block on issues.
    - randrews 7y ago
      Yep. I've worked on great teams that did all those things. And not-so-great teams that didn't. The latter is really draining.
  - Stratoscope 7y ago
    I have suffered under far too many "this is how I would have written it" reviews. It can be really demoralizing to write perfectly good, working, readable, performant, well-tested code only to have it rejected and have to rewrite it because the reviewer wishes you had used a different C++ feature. I tried something like your "I feel comfortable maintaining this" approach recently when I was the reviewer. We have an internal geodata visualization tool that I've done some work on lately, and a talented C++ developer added a cool and useful new feature to the JavaScript front end. He asked me to review it and said, "I'm sure you will find a lot of things I should change! This is my first real JavaScript project other than some hobby stuff." And he was right. There was a lot of stuff I would have done differently. There was a mix of jQuery and document.getElementById, a fair amount of repeated code, var instead of const or let, and so on. Our "house style" for reviews would have been for me to comment on every single one of these things line by line and expect him to change them all to my satisfaction. It would then require a second review pass to clear up misunderstandings, and a third to see if it all ended up OK. Instead I told him: "Your new feature is awesome! You're right, there are a lot of nitpicky things I would change in the code to bring it up to more modern JS style, and I'm glad you asked about that. But I didn't find anything that looks broken or dangerous. And the feature works, right? I could spend half a day writing comments explaining everything I would do differently, and that would keep you busy for another half day fixing it up. But I know you have more important things to work on right now, so maybe we can try something different. Go ahead and submit the code as is so people can start using it. When I get a little time I will just go ahead and make the changes I'm thinking of. I'll add review comments on my own changes and send it to you for review so you can see what I changed and why. That will avoid a lot of back-and-forth. I think it will save us both a lot of time and be a more pleasant experience too." Needless to say, the developer liked this idea. And his manager was within earshot when we discussed it and thanked me for thinking of this approach. I still haven't made that update, and this reminds me to do it sometime soon. But the code still works, people are using it productively, it has not failed once, and who really cares if there are some minor imperfections in the code style? None of our code is perfect!
    - blub 7y ago
      You'll never do that update right? The more you wait, the more that developer will get annoyed when you interrupt them with it. :) In this case actually I would have taken the half an hour and sat with them to explain what I think should be done differently and why, prioritized by importance. Reviews done through tools work if there aren't many comments and if the basic design is ok. I've seen reviews where someone's not skilled enough and they have to be taught idioms in dozens of comments in 10-20 patch sets. It's pretty horrible, but the positive side is that at least they're not merging crap.
blub 7y ago
Code review is one of those things where the culture around reviewing and the experience of the reviewers makes all the difference. A code review where the boss reviews code is pathological. When experienced peers do the review it can be brilliant. In fact I would only trust a team of very competent seniors not to need code review. Even then, getting another opinion on your code can be worth it.
- atq2119 7y ago
  Everybody needs code review. Though the way I scan the code during review does change by how much I know and respect the author on a technical level. That correlates with but is not identical to their formal seniority.
ambicapter 7y ago
I don't think are any good work techniques when you're surrounded by assholes.

I would add code reviews to that. The advent of those was when it really stopped being fun for me, because it's really hard to keep that from turning into "your boss nitpicking every line you write."

oriolid 7y ago

In my opinion this is much, much better than trying to decipher what the heck that one co-worker pushed to production and how exactly it broke things. Of course there are ways to smuggle rubbish through code review, but it's still better than no reviews.

randrews 7y ago
Both have problems. The best system is one where a passing review means "I feel comfortable maintaining this;" too often a passing review means "this is how I would have written it."
- Silhouette 7y ago
  IMHO, review processes should concentrate on identifying problems, not solutions.
- wes-k 7y ago
  Agreed. Few things to consider: - linters to help move away from code style comments - Peer code reviews (if possible) - inform your team/boss of bike shedding - use a tool like reviewable.io where comments can be marked as blocking or not. I’ll often comment on preferences and block on issues.
  - randrews 7y ago
    Yep. I've worked on great teams that did all those things. And not-so-great teams that didn't. The latter is really draining.
- Stratoscope 7y ago
  I have suffered under far too many "this is how I would have written it" reviews. It can be really demoralizing to write perfectly good, working, readable, performant, well-tested code only to have it rejected and have to rewrite it because the reviewer wishes you had used a different C++ feature. I tried something like your "I feel comfortable maintaining this" approach recently when I was the reviewer. We have an internal geodata visualization tool that I've done some work on lately, and a talented C++ developer added a cool and useful new feature to the JavaScript front end. He asked me to review it and said, "I'm sure you will find a lot of things I should change! This is my first real JavaScript project other than some hobby stuff." And he was right. There was a lot of stuff I would have done differently. There was a mix of jQuery and document.getElementById, a fair amount of repeated code, var instead of const or let, and so on. Our "house style" for reviews would have been for me to comment on every single one of these things line by line and expect him to change them all to my satisfaction. It would then require a second review pass to clear up misunderstandings, and a third to see if it all ended up OK. Instead I told him: "Your new feature is awesome! You're right, there are a lot of nitpicky things I would change in the code to bring it up to more modern JS style, and I'm glad you asked about that. But I didn't find anything that looks broken or dangerous. And the feature works, right? I could spend half a day writing comments explaining everything I would do differently, and that would keep you busy for another half day fixing it up. But I know you have more important things to work on right now, so maybe we can try something different. Go ahead and submit the code as is so people can start using it. When I get a little time I will just go ahead and make the changes I'm thinking of. I'll add review comments on my own changes and send it to you for review so you can see what I changed and why. That will avoid a lot of back-and-forth. I think it will save us both a lot of time and be a more pleasant experience too." Needless to say, the developer liked this idea. And his manager was within earshot when we discussed it and thanked me for thinking of this approach. I still haven't made that update, and this reminds me to do it sometime soon. But the code still works, people are using it productively, it has not failed once, and who really cares if there are some minor imperfections in the code style? None of our code is perfect!
  - blub 7y ago
    You'll never do that update right? The more you wait, the more that developer will get annoyed when you interrupt them with it. :) In this case actually I would have taken the half an hour and sat with them to explain what I think should be done differently and why, prioritized by importance. Reviews done through tools work if there aren't many comments and if the basic design is ok. I've seen reviews where someone's not skilled enough and they have to be taught idioms in dozens of comments in 10-20 patch sets. It's pretty horrible, but the positive side is that at least they're not merging crap.

blub 7y ago

Code review is one of those things where the culture around reviewing and the experience of the reviewers makes all the difference. A code review where the boss reviews code is pathological. When experienced peers do the review it can be brilliant. In fact I would only trust a team of very competent seniors not to need code review. Even then, getting another opinion on your code can be worth it.

atq2119 7y ago
Everybody needs code review. Though the way I scan the code during review does change by how much I know and respect the author on a technical level. That correlates with but is not identical to their formal seniority.

ambicapter 7y ago

I don't think are any good work techniques when you're surrounded by assholes.