43 ms·

What's wrong with giving exactly that feedback? "The main logic is too monolithic and deeply nested. Please refactor in a more maintainable version, split into

by rusanu 11y ago

What's wrong with giving exactly that feedback? "The main logic is too monolithic and deeply nested. Please refactor in a more maintainable version, split into several simpler methods". Unless there is some politik behind your question, I see no reason to say exactly what you feel. I have given (and received!) similar reviews many times. Btw I don't think is useful to do the rewrite yourself in the review: you are robbing the original author of a chance to redeem him/her self. I also think that refactoring a complex function is not a "rewrite from scratch", and even a rewrite from scratch on a second iteration is seldom as time consuming/complicated as the first iteration. If you or your coworkers shy away from giving honest feedback in code reviews you have a big problem. Lowering the bar leads to technical debt grow, which someone will have to deal with later. You are missing a great opportunity to educate (and learn from) your peers. If your managers pressure you or your coworkers to "go easy" on the quality (ie. "It works, let him check it in!") then again you have a big problem. You trade immediate achievement for later pain. It may be because is their immediate achievement but somebody else's pain later, and in such a case you're in a bad environment and not much you can do. If you personally want to avoid the friction of a review that is asking for a major rewrite, you can sugar coat it, but ultimately giving negative review (when warranted!) is part of your daily job. As long as is objective and arguably leads to better code (and behavior) it should offend none. Keep in mind that we all often write the code in iterations, and once is working we feel an urge to push it out without taking a break and looking over it again, but you the code reviewer see directly the end result. If is not pretty, is not necessarily because the original author is a bad programmer. It may simply require to take a break and have a look again, and he may see the same problem are obvious to you.

maratd 11y ago
> What's wrong with giving exactly that feedback? Giving feedback on a code review is really meant to improve small bits here and there. Incremental improvement. If someone doesn't know a new language construct, a better way of doing things, etc. If someone simply writes crap, telling them that they wrote crap isn't helpful ... because they're clearly incompetent and are only capable of writing crap. > If your managers pressure you or your coworkers to "go easy" on the quality (ie. "It works, let him check it in!") then again you have a big problem. This is really the issue. First, you'll piss off the incompetent coder. Second, you'll stall the project. Third, you'll piss off your boss, since he's probably the guy who hired the incompetent coder. All the while, you're "making waves". All based on what is essentially your opinion on what clean code should look like. Because it does run. I would just take it slow and incrementally target some of the worst offenses. You'll never make the guy into a competent developer, but you can at least ameliorate some of the worst offenses and improve the quality of the code. If you're actually working on the codebase as well, instead of just reviewing it ... I would talk to management. Explain the situation. They're either going to fire the new guy or you're going to need to find another job. One of the reasons I like freelancing is that I don't have to face these dilemmas. The next client is just around the corner.
- rancur 11y ago
  > you're making waves as long as nobody makes waves, we'll have a peaceful experience until the shitstorm of the reality of unmaintainable code hits. this is one reason why companies need to die, and not be bailed out
- curiousDog 11y ago
  > If someone simply writes crap, telling them that they wrote crap isn't helpful ... because they're clearly incompetent and are only capable of writing crap. I think you can't reach such a conclusion without knowing the circumstances. What if he's a fresh college grad or a junior developer? I think you're coming from a start-up environment where everyone around you is supposed to be an expert in their field. Most Big Cos hire fresh college grads and mentor them. So mistakes like this are common. Now considering he's a junior, Remus's advice is spot on. I helped a dev on our team in a similar way who went on to write some really complicated distributed systems code that you use today. Also Remus comes from MSFT where this advice would apply.
  - maratd 11y ago
    > What if he's a fresh college grad or a junior developer? Even when I was in college, I didn't write crap. Obviously my code wasn't great, but it wasn't crap. And there is a lot of crap out there, I see it every day. Based on the OP's description, we're talking about someone who isn't capable of producing quality work. You can either write code or you can't and it is very obvious by seeing the code. If the person can write code, but needs a few pointers, that's what code reviews are for. If they can't write code, they need to move on to a different profession ... or move up to management.
    - DigitalJack 11y ago
      That's BS. Coding isn't some kind of magical incantation, in spite of the jokes in SICP. Anyone that can understand basic algebra can learn to code.
      - 11y ago

by rusanu 11y ago

maratd 11y ago

> What's wrong with giving exactly that feedback? Giving feedback on a code review is really meant to improve small bits here and there. Incremental improvement. If someone doesn't know a new language construct, a better way of doing things, etc. If someone simply writes crap, telling them that they wrote crap isn't helpful ... because they're clearly incompetent and are only capable of writing crap. > If your managers pressure you or your coworkers to "go easy" on the quality (ie. "It works, let him check it in!") then again you have a big problem. This is really the issue. First, you'll piss off the incompetent coder. Second, you'll stall the project. Third, you'll piss off your boss, since he's probably the guy who hired the incompetent coder. All the while, you're "making waves". All based on what is essentially your opinion on what clean code should look like. Because it does run. I would just take it slow and incrementally target some of the worst offenses. You'll never make the guy into a competent developer, but you can at least ameliorate some of the worst offenses and improve the quality of the code. If you're actually working on the codebase as well, instead of just reviewing it ... I would talk to management. Explain the situation. They're either going to fire the new guy or you're going to need to find another job. One of the reasons I like freelancing is that I don't have to face these dilemmas. The next client is just around the corner.

rancur 11y ago
> you're making waves as long as nobody makes waves, we'll have a peaceful experience until the shitstorm of the reality of unmaintainable code hits. this is one reason why companies need to die, and not be bailed out
curiousDog 11y ago
> If someone simply writes crap, telling them that they wrote crap isn't helpful ... because they're clearly incompetent and are only capable of writing crap. I think you can't reach such a conclusion without knowing the circumstances. What if he's a fresh college grad or a junior developer? I think you're coming from a start-up environment where everyone around you is supposed to be an expert in their field. Most Big Cos hire fresh college grads and mentor them. So mistakes like this are common. Now considering he's a junior, Remus's advice is spot on. I helped a dev on our team in a similar way who went on to write some really complicated distributed systems code that you use today. Also Remus comes from MSFT where this advice would apply.
- maratd 11y ago
  > What if he's a fresh college grad or a junior developer? Even when I was in college, I didn't write crap. Obviously my code wasn't great, but it wasn't crap. And there is a lot of crap out there, I see it every day. Based on the OP's description, we're talking about someone who isn't capable of producing quality work. You can either write code or you can't and it is very obvious by seeing the code. If the person can write code, but needs a few pointers, that's what code reviews are for. If they can't write code, they need to move on to a different profession ... or move up to management.
  - DigitalJack 11y ago
    That's BS. Coding isn't some kind of magical incantation, in spite of the jokes in SICP. Anyone that can understand basic algebra can learn to code.
    - 11y ago