4 ms·

The issue I run into with this comes part of the code review. So some developer is going to fix some code, and the code is ugly. So the developer fixes the bug

The issue I run into with this comes part of the code review. So some developer is going to fix some code, and the code is ugly. So the developer fixes the bug and makes the code prettier and push it as a single commit. I am then left with figuring out which changes are related to the bug fix and what is related to the cleanup. If I tell the developer to first fix the bug and then cleanup in a separate commit, it may be inefficient since the fix after cleanup may not be the same as fix before cleanup. It may also lead to the cleanup being skipped because his work is technically done after the fix. If I tell him to cleanup first and fix later, it's also seem to lead to issues since the developer has typically already figured out how to fix it since he came into the code with the mindset of fixing the bug. Of course I like the code being cleaned up, and I like the idea that you improve the code as you work with it. But I'm not sure what the best method is. Maybe the issue is with me, that I want the cleanup and fix in separate commits for easier reviewing. Personally if I spot ugly code when fixing a bug I add unit tests, fix the bug and then add another ticket to improve the code. I then plan that code cleanup for this or the next sprint. But it feels tedious and is probably inefficient as well

goalieca 6y ago
> I am then left with figuring out which changes are related to the bug fix and what is related to the cleanup. Hmm. That has never been an issue for me in practice unless they go way too far with the refactoring. Usually it’s me who suggests cleanup for defensive coding reasons because the bug is often a coding issue.
- hinkley 6y ago
  People frustrate me by taking the short view of these things. Bigger refactorings and architectural changes usually require tracing the tendrils of an assumption out through the code. It’s not a matter of “can they figure it out,” it’s a matter of doing that in ten places and still somehow hold the other three bits of critical data in short term memory. If this path isn’t fruitful then there are five other theories to try so it’s not particularly amenable to writing it down. It’s flow state depth first search, and the crappy non-idiomatic code is ruining the flow. People always try to deflect based on some intelligence argument. I’m not trying to understand your code, I’m trying to understand something you missed. This isn’t about intelligence, it’s about wisdom.
karmakaze 6y ago
Solution is to merge one git branch with three commits: new feature+new tests, (optional) new tests for refactor, refactor. I do this using git reset --soft when I'm done, even if it means having to split up changes made to the same file across commits. Save the original work and diff new complete changes with original set.
hinkley 6y ago
Yes, you absolutely ask them to do the refactor first. Because that’s how grownups code. If they want to mature as programmers, they need to think about longevity of the code. You can’t just keep spackling over problems again and again and expect any sort of integrity to remain. Make the change easy, then make the easy change. If they don’t they should expect comments. Code reviews are at least as much about setting standards and expectations as they are about catching new bugs.
- tuwtuwtuwtuw 6y ago
  I think it would be wrong to set such a hard rule. You may have a bug which you could fix in say 2 hours but refactoring the code as a whole take 5 days. Is it always the "grownup" way to tell the customer that they can't use the system the next 3 days? I think it sometimes makes more sense to fix the code as is and then schedule a refactoring.
  - hinkley 6y ago
    Mastery is not about learning the rules from a book. It’s about learning the rules that no one wrote down, and about learning which things that were written down aren’t rules so much as guidelines. (This is the exact story arc of probably 30% of all martial arts movies). The quick hacks are quick. That’s why they are so addicting. You have to look at the bigger picture. Is this the third time you’ve used the same quick fix? Then there’s a deeper problem. Does this problem keep happening in production? Then it’s better to take a breath, fix it correctly and once and for all, rather than to be randomly pre-empted by this issue for years to come. Those liabilities stack up, and eventually form clusters that make us look like we can’t do the job. And last but not least, does this person really keep their promises to come back and fix things later, or is it just pretty words? If Cat Stevens were a coder, he’d write a song about all the someday lies developers tell themselves and each other. Father-son neglect dynamics have nothing on code rot.

goalieca 6y ago

> I am then left with figuring out which changes are related to the bug fix and what is related to the cleanup. Hmm. That has never been an issue for me in practice unless they go way too far with the refactoring. Usually it’s me who suggests cleanup for defensive coding reasons because the bug is often a coding issue.

hinkley 6y ago
People frustrate me by taking the short view of these things. Bigger refactorings and architectural changes usually require tracing the tendrils of an assumption out through the code. It’s not a matter of “can they figure it out,” it’s a matter of doing that in ten places and still somehow hold the other three bits of critical data in short term memory. If this path isn’t fruitful then there are five other theories to try so it’s not particularly amenable to writing it down. It’s flow state depth first search, and the crappy non-idiomatic code is ruining the flow. People always try to deflect based on some intelligence argument. I’m not trying to understand your code, I’m trying to understand something you missed. This isn’t about intelligence, it’s about wisdom.

karmakaze 6y ago

Solution is to merge one git branch with three commits: new feature+new tests, (optional) new tests for refactor, refactor. I do this using git reset --soft when I'm done, even if it means having to split up changes made to the same file across commits. Save the original work and diff new complete changes with original set.

hinkley 6y ago

Yes, you absolutely ask them to do the refactor first. Because that’s how grownups code. If they want to mature as programmers, they need to think about longevity of the code. You can’t just keep spackling over problems again and again and expect any sort of integrity to remain. Make the change easy, then make the easy change. If they don’t they should expect comments. Code reviews are at least as much about setting standards and expectations as they are about catching new bugs.

tuwtuwtuwtuw 6y ago
I think it would be wrong to set such a hard rule. You may have a bug which you could fix in say 2 hours but refactoring the code as a whole take 5 days. Is it always the "grownup" way to tell the customer that they can't use the system the next 3 days? I think it sometimes makes more sense to fix the code as is and then schedule a refactoring.
- hinkley 6y ago
  Mastery is not about learning the rules from a book. It’s about learning the rules that no one wrote down, and about learning which things that were written down aren’t rules so much as guidelines. (This is the exact story arc of probably 30% of all martial arts movies). The quick hacks are quick. That’s why they are so addicting. You have to look at the bigger picture. Is this the third time you’ve used the same quick fix? Then there’s a deeper problem. Does this problem keep happening in production? Then it’s better to take a breath, fix it correctly and once and for all, rather than to be randomly pre-empted by this issue for years to come. Those liabilities stack up, and eventually form clusters that make us look like we can’t do the job. And last but not least, does this person really keep their promises to come back and fix things later, or is it just pretty words? If Cat Stevens were a coder, he’d write a song about all the someday lies developers tell themselves and each other. Father-son neglect dynamics have nothing on code rot.