4 ms·

In general I believe this to be dangerous advice, particularly conducting a drive by refactoring. If you did not write it in the first place then you did not u

In general I believe this to be dangerous advice, particularly conducting a drive by refactoring. If you did not write it in the first place then you did not understand the mental model or tradeoffs that were made. A year from now when there is a critical bug attached to your commit but rather than a two line change it is several pages of diffs, how will the next person figure out what you did?

mooreds 6y ago
Interesting perspective. Perhaps I should have been more precise that you don't want to make the code worse, and that you should strive to make sure your "improvement" doesn't do so. I thought that was implied, but perhaps should make it explicit. I've seen a lot of code that was confusing left alone because "hey it works" and slowly rot, even as people are in and out of it.
- uberman 6y ago
  It is exactly the code that is confusing but seems to work that I fear a junior dev will refactor (with the best of intentions) in a drive by. To be candid, I am guilty of drive by refactoring as well as refactoring in the context of a bug fix commit as well as being forced to review my own commits that resulted in bugs and had both fixes and refactoring in them. I now "let sleeping dogs lie" as they say, unless my task is specifically to refactor/reimplement. I try to incorporate this quote into the work I do and the code reviews I conduct/mentor with more junior devs. Chesterton’s Fence, G. K. Chesterton There exists in such a case a certain institution or law; let us say, for the sake of simplicity, a fence or gate erected across a road. The more modern type of reformer goes gaily up to it and says, “I don’t see the use of this; let us clear it away.” To which the more intelligent type of reformer will do well to answer: “If you don’t see the use of it, I certainly won’t let you clear it away. Go away and think. Then, when you can come back and tell me that you do see the use of it, I may allow you to destroy it This may not be exactly applicable here, but I like it none the less. Perhaps more succinctly "If it ain't broke, don't fix it"
  - mooreds 6y ago
    Hah! I just had a friend drop a reference to Chesterton's Fence on twitter the other day. I think that this is definitely a great point. Time boxing and code review and cultural guidance are probably the best defense. But I think a valid thing to do if a newer dev doesn't understand a section of code is to ask why and see if there are steps short of refactoring to improve that (comments, docs). What I hate and strive to avoid is code that is "abandon hope, all ye who enter" territory--totally a mess but "works" and no one wants to touch it. All in all I think we agree, drive by refactoring is the flip side of that and can lead to subtle bugs a year from now. It's a balance. EDIT: I'll update the post to reflect this discussion. Thanks!
    - uberman 6y ago
      I really enjoyed this discussion and will follow your blog. Thanks.

mooreds 6y ago

Interesting perspective. Perhaps I should have been more precise that you don't want to make the code worse, and that you should strive to make sure your "improvement" doesn't do so. I thought that was implied, but perhaps should make it explicit. I've seen a lot of code that was confusing left alone because "hey it works" and slowly rot, even as people are in and out of it.

uberman 6y ago
It is exactly the code that is confusing but seems to work that I fear a junior dev will refactor (with the best of intentions) in a drive by. To be candid, I am guilty of drive by refactoring as well as refactoring in the context of a bug fix commit as well as being forced to review my own commits that resulted in bugs and had both fixes and refactoring in them. I now "let sleeping dogs lie" as they say, unless my task is specifically to refactor/reimplement. I try to incorporate this quote into the work I do and the code reviews I conduct/mentor with more junior devs. Chesterton’s Fence, G. K. Chesterton There exists in such a case a certain institution or law; let us say, for the sake of simplicity, a fence or gate erected across a road. The more modern type of reformer goes gaily up to it and says, “I don’t see the use of this; let us clear it away.” To which the more intelligent type of reformer will do well to answer: “If you don’t see the use of it, I certainly won’t let you clear it away. Go away and think. Then, when you can come back and tell me that you do see the use of it, I may allow you to destroy it This may not be exactly applicable here, but I like it none the less. Perhaps more succinctly "If it ain't broke, don't fix it"
- mooreds 6y ago
  Hah! I just had a friend drop a reference to Chesterton's Fence on twitter the other day. I think that this is definitely a great point. Time boxing and code review and cultural guidance are probably the best defense. But I think a valid thing to do if a newer dev doesn't understand a section of code is to ask why and see if there are steps short of refactoring to improve that (comments, docs). What I hate and strive to avoid is code that is "abandon hope, all ye who enter" territory--totally a mess but "works" and no one wants to touch it. All in all I think we agree, drive by refactoring is the flip side of that and can lead to subtle bugs a year from now. It's a balance. EDIT: I'll update the post to reflect this discussion. Thanks!
  - uberman 6y ago
    I really enjoyed this discussion and will follow your blog. Thanks.