4 ms·

Some sensible ideas - however, I'm on the fence about this one: "Unhelpful behavior: asking engineers to solve problems they didn’t cause" Generally, I agree w

by nartz 9y ago

Some sensible ideas - however, I'm on the fence about this one: "Unhelpful behavior: asking engineers to solve problems they didn’t cause" Generally, I agree with breaking out bugs or issues that are found 'along the way' into separate subtasks or tickets. However, often what happens is a ticket is thrown into the backlog and not addressed. Because the developer has just touched the code, this is often the best time to refactor as its still fresh in the mind, and this creates a culture of constant improvement. Of course everything depends, in this case, the complexity of the fix is the main limiting factor. We truly believe in Refactoring along the way instead of trying to break out tech debt stories separately.

microtherion 9y ago
Yes, I've been on the fence about this one as well. What I generally do is only add such fix-it comments if I identify other issues in my review that WERE caused by the current PR and need to be addressed anyway. So I would not hold up a PR for such matters, but if it is otherwise incomplete, I feel it's OK to ask for a modest amount of extra work.
xg15 9y ago
Seems to me, this is more a problem of ticket/workload organisation. > However, often what happens is a ticket is thrown into the backlog and not addressed. Every management method is different, but e.g. in Scrum (I think), there is the idea of a person reviewing and priorizing work tasks according to the roadmap. this person would decide if a "fix this messy code" task is important enough or not. Then again, I think there is the every day a good deed philosophy of refactoring, too. I think in that case, one should just make sure that the "fix along the way" task doesn't disturb the original problems too much (so commits can still be reviewed) and that there are no unintended side-effects.
thedz 9y ago
I agree with refactoring along the way, but I think it's important to clearly state the _purpose_ of a branch or pull request, and to make sure that refacors don't wildly blow out the scope of the PR. I've been on the receiving end of too many PRs that refactor a bunch of things all at once because "well I was in there" to feel good about unchecked scope creep in PRs.
misterbowfinger 9y ago
It's a conversation. Sometimes, it's no big deal, and sometimes, it's fine to deflect and say "I'll make a ticket for it".
kelnos 9y ago
I'm a little torn on this as well, but I think I'm in the author's camp: 1. The person making the change to the code for their purpose may not have the required overall understanding to execute a refactor. 2. I believe in a One Change At A Time policy: I'd like to be able to, at least in theory, measure the effects of any change. If you slap two changes together, that becomes difficult or impossible. 3. The person making the change has a purpose in making that change, and likely has committed to some sort of schedule or timeline for getting the work done. Forcing them to play janitor could easily derail that. 4. The change at hand might just be a small piece in a larger body of work, and shifting focus to a refactor would be disruptive. Having said that, I do often take the initiative in doing refactors when making a change, if it makes sense to me to do so. But I don't think it makes sense to push that expectation on others. Hell, it's possible/likely that the person making the change has noticed the opportunity for a refactor, but has decided against it for whatever reason. I might suggest a refactor in a code review, but couched with language that it's entirely optional, and won't block merge of the change as-is if the author decides it's not the right call for them.
- le-mark 9y ago
  I believe in a One Change At A Time policy: I'd like to be able to, at least in theory, measure the effects of any change. If you slap two changes together, that becomes difficult or impossible. One change at a time has the benefit of not polluting traceability of what was done and why, in the ticketing system. If a commit for ticket1234 was to fix a bug in the payment system, and the commit has unrelated code fixing up user data access, that's bad in my opinion. I've found myself in many code bases where the only trail of breadcrumbs I've had is ticket references in the commit messages. When these aren't reliable, you're left with not much at all.

by nartz 9y ago

microtherion 9y ago

Yes, I've been on the fence about this one as well. What I generally do is only add such fix-it comments if I identify other issues in my review that WERE caused by the current PR and need to be addressed anyway. So I would not hold up a PR for such matters, but if it is otherwise incomplete, I feel it's OK to ask for a modest amount of extra work.

xg15 9y ago

Seems to me, this is more a problem of ticket/workload organisation. > However, often what happens is a ticket is thrown into the backlog and not addressed. Every management method is different, but e.g. in Scrum (I think), there is the idea of a person reviewing and priorizing work tasks according to the roadmap. this person would decide if a "fix this messy code" task is important enough or not. Then again, I think there is the every day a good deed philosophy of refactoring, too. I think in that case, one should just make sure that the "fix along the way" task doesn't disturb the original problems too much (so commits can still be reviewed) and that there are no unintended side-effects.

thedz 9y ago

I agree with refactoring along the way, but I think it's important to clearly state the _purpose_ of a branch or pull request, and to make sure that refacors don't wildly blow out the scope of the PR. I've been on the receiving end of too many PRs that refactor a bunch of things all at once because "well I was in there" to feel good about unchecked scope creep in PRs.

misterbowfinger 9y ago

It's a conversation. Sometimes, it's no big deal, and sometimes, it's fine to deflect and say "I'll make a ticket for it".

kelnos 9y ago

I'm a little torn on this as well, but I think I'm in the author's camp: 1. The person making the change to the code for their purpose may not have the required overall understanding to execute a refactor. 2. I believe in a One Change At A Time policy: I'd like to be able to, at least in theory, measure the effects of any change. If you slap two changes together, that becomes difficult or impossible. 3. The person making the change has a purpose in making that change, and likely has committed to some sort of schedule or timeline for getting the work done. Forcing them to play janitor could easily derail that. 4. The change at hand might just be a small piece in a larger body of work, and shifting focus to a refactor would be disruptive. Having said that, I do often take the initiative in doing refactors when making a change, if it makes sense to me to do so. But I don't think it makes sense to push that expectation on others. Hell, it's possible/likely that the person making the change has noticed the opportunity for a refactor, but has decided against it for whatever reason. I might suggest a refactor in a code review, but couched with language that it's entirely optional, and won't block merge of the change as-is if the author decides it's not the right call for them.

le-mark 9y ago
I believe in a One Change At A Time policy: I'd like to be able to, at least in theory, measure the effects of any change. If you slap two changes together, that becomes difficult or impossible. One change at a time has the benefit of not polluting traceability of what was done and why, in the ticketing system. If a commit for ticket1234 was to fix a bug in the payment system, and the commit has unrelated code fixing up user data access, that's bad in my opinion. I've found myself in many code bases where the only trail of breadcrumbs I've had is ticket references in the commit messages. When these aren't reliable, you're left with not much at all.