7 ms·

I frequently find myself drastically refactoring code to understand it. I don't commit those changes because it's not worth the effort to justify the cleanup to

by lugg 8y ago

I frequently find myself drastically refactoring code to understand it. I don't commit those changes because it's not worth the effort to justify the cleanup to people who treat these rules as gospel. Apparently me spending half a day reading code is no big deal but cleaning it up is a waste of time. Shrug.

quietbritishjim 8y ago
I think the objections to rewriting code are sometimes justified, rather than just blindly following rules. As you said, refactoring code helps you understand it. This means that you can end up feeling like your code is objectively clearer than before you started, but sometimes it's just an illusion caused by the fact that you just (re)wrote it. If there are other people in the company that already understand that functionality, you're disrupting the effort they put into understanding the old version of the code. Combine this with the risk of introducing bugs or missing some obscure bits of functionality, and there is valid reason to object. I find this is particularly common mistake by junior programmers (not saying this applies to you), presumably because they aren't used to reading other people's code. Frustratingly, this is often coupled with an attitude that missing out large chunks of existing user functionality is acceptable if it makes the code a bit simpler. Of course, sometimes rewrites/refactors really are an improvement. Sometimes code really is fragile and confusing, either because of who wrote it or because it has had many small changes tacked on in the easiest places. Or perhaps the last person that understood that code has left the company, so it's OK that you find the code clearer only because you just wrote it! But in any case, it is fair to ask for real justification for a rewrite.
- logicallee 8y ago
  (Genuine question.) Do you think "small changes" shouldn't be "tacked on in the easiest places"? I'll try to give an example I hope is realistic: One of the heaviest things an application can get is a complete theme system. Suppose you don't have one. It's not a requirement. Adding a theme system when there is none is months of work and might impact basically every line of code that displays anything. So you're not doing it. Now for some exceptional system there is just one case where somewhere you are displaying something under an external widget that doesn't meet its size constraints or whatever - long story short your text is invisible, you want to inverse the font to get it working. You don't have any code like that. Do you think it is OK to add it to the easiest possible place: in this case perhaps you add an optional argument called "need_to_invert_color" (this awkward phrasing tells you it's a hack) to a single function, default it as false, comment it as: //invert the color of the font. This is needed where an external graphing widget with a black background leaks onto our canvas due to not respecting our pixel boundaries, so that our text displays over it. And then where you call comment the same thing, that //currently a bug in the widget code makes the widget leak xyz pixels below its bottom border. As a temporary fix we introduce an argument need_to_invert_color into our display function. As of this writing 3 Jan 2019 we are just using it from here. The correct fix would be for the widget to stop leaking instead, and when that is done white text is unnecessary - and we might not notice. So we start by testing whether the area we will be overlaid over is indeed the wrong color. ETC. In other words a quick hack for a corner case, that doesn't fix the underlying bug (workaround) and even as a hack makes use of something that doesn't exist (a theme system), instead adding and documenting a half-assed thing tacked on.
  - quietbritishjim 8y ago
    Of course this is just my opinion, but in many situations I think tacking one small change on to the easiest place is fine. My comment earlier wasn't meant as a criticism of that practice. It's just I would consider refactoring after this happens multiple times: at some point, all those little tweaks can add up enough that the original design of the "core" code gets lost in the noise. When exactly is the right time can be hard to figure out. Especially because, if enough hacks have accumulated that code really does need a reshuffle, then the job of refactoring is harder, which actually increases the temptation to put it off. But I wouldn't make a big sweeping change just because of one small hack that's a bit ugly.
    - logicallee 8y ago
      Thanks, this answers my question. I had always thought documenting half-assed solutions was particularly important not just for anyone looking at the code (to understand the whole code quickly, if they're new to it, make changes, etc - or even if the author comes back 6 months later, to be reminded of the assumptions that led to the hacks) but because whoever finally sits down to architect the rewrite will have some indications of the "hacks" (degrees of freedom) that must be made possible in the proper version. I thought they (or I) would sit down, have all the documented hacks on one side of their table, then use them to architect a nice solution that the hacks can then use instead, kind of like a requirements document. This was my thinking on a lot of projects. But you know what? That rewrite never became necessary! So not only did I not start with the "right" architecture - I didn't end with it either! That's what "many small hacks in the easiest of places" reminded me of and I wondered if you in fact do approve of it. It has always seemed fine for me. Just no problem at all. But with documentation right there and the worse the hack, the clearer the documentation right there explaining and justifying it, up to and including "I don't know why this works but this system call makes the next line succeed, whereas removing it causes the next line to fail sometimes - this is tested in testxzy." Obviously a hack, a terrible hack if you don't know why it works. And a project can end up with a lot of these.
- lugg 8y ago
  You're talking about rewrites. I'm talking about cleaning up messes left by lazy people long ago, so long ago that most of the juniors are scared to clean it up despite it being impossible to understand without a days reading. You can say it disrupts others understanding of shitty old code, frankly I don't care. Code is not immutable and maintaining status quo helps nobody except the old guard stay relevant.
  - quietbritishjim 8y ago
    I said: > Of course, sometimes rewrites/refactors really are an improvement If that's what you were talking about in the first place, fair enough and my apologies.
    - lugg 8y ago
      No apologies needed. It was a fair assumption and I do agree with your points, although I think I've come to move more in the other direction the last few years. Those perfect one line fixes are great but they approach their limits and eventually need to be refactored. This is natural and should not be frowned upon. Likewise you can't just refactor all the time, one line fixes are faster and less costly in all sorts of ways. The key is to pick and choose when to use each strategy. I think we're too often looking for simple rules. There are none. Just a bunch of guidelines.
z0r 8y ago
Writing code is hard, so there's nothing wrong with reading it being hard too. (I'm only half kidding).
zorked 8y ago
Five people understand the system. You refactor it. Now one person understands the system.
- Aeolun 8y ago
  Initially the system took 8 hours of reading and 2 days of refactoring to grasp. Now it takes 2 hours. I’ll take that.
chucksmash 8y ago
Working Effectively with Legacy Code recommends just this idea. They call it scratch refactoring. Refactor without a lot of forethought to see how the system works, then just throw away your changes but keep your newfound understanding.
- lugg 8y ago
  This is effectively "it." But I do like to branch off the more useful refactors into their own commits.
pault 8y ago
I often see some bit of code, or a dependency in a project that I think is over engineered and too complex and start rewriting; first I get something basic working, then I discover some edge case, then another, and another, and eventually I realize that I have reimplemented the original code. It always makes me feel silly, but at the same time those have been the best learning experiences I've ever had.
- zozbot123 8y ago
  The proper response to that is to actively document these edge cases (within the original code) so that they don't get missed as the code is maintained in the future.
  - pault 8y ago
    I do, so I can remember how it works when I see it again in six months. :)
titzer 8y ago
Take a moment to consider that someone else in your team might actually understand the code in question, and when you rewrite it (and commit), you destroy their knowledge about how it works. Sure, from your perspective, you made the code better, easier to understand. From their perspective, even if they do the code review themselves, they will not understand it as thoroughly as you do, and they will be burdened with both the knowledge of how it used to be, and how it is now. The net result can be negative, and eventually no one understands any code that they didn't refactor in the last ~6 months, because if it's any older than that, someone's rewritten it. Sometimes you gotta sit down and bite the bullet and read and ask questions.

by lugg 8y ago

quietbritishjim 8y ago

I think the objections to rewriting code are sometimes justified, rather than just blindly following rules. As you said, refactoring code helps you understand it. This means that you can end up feeling like your code is objectively clearer than before you started, but sometimes it's just an illusion caused by the fact that you just (re)wrote it. If there are other people in the company that already understand that functionality, you're disrupting the effort they put into understanding the old version of the code. Combine this with the risk of introducing bugs or missing some obscure bits of functionality, and there is valid reason to object. I find this is particularly common mistake by junior programmers (not saying this applies to you), presumably because they aren't used to reading other people's code. Frustratingly, this is often coupled with an attitude that missing out large chunks of existing user functionality is acceptable if it makes the code a bit simpler. Of course, sometimes rewrites/refactors really are an improvement. Sometimes code really is fragile and confusing, either because of who wrote it or because it has had many small changes tacked on in the easiest places. Or perhaps the last person that understood that code has left the company, so it's OK that you find the code clearer only because you just wrote it! But in any case, it is fair to ask for real justification for a rewrite.

logicallee 8y ago
(Genuine question.) Do you think "small changes" shouldn't be "tacked on in the easiest places"? I'll try to give an example I hope is realistic: One of the heaviest things an application can get is a complete theme system. Suppose you don't have one. It's not a requirement. Adding a theme system when there is none is months of work and might impact basically every line of code that displays anything. So you're not doing it. Now for some exceptional system there is just one case where somewhere you are displaying something under an external widget that doesn't meet its size constraints or whatever - long story short your text is invisible, you want to inverse the font to get it working. You don't have any code like that. Do you think it is OK to add it to the easiest possible place: in this case perhaps you add an optional argument called "need_to_invert_color" (this awkward phrasing tells you it's a hack) to a single function, default it as false, comment it as: //invert the color of the font. This is needed where an external graphing widget with a black background leaks onto our canvas due to not respecting our pixel boundaries, so that our text displays over it. And then where you call comment the same thing, that //currently a bug in the widget code makes the widget leak xyz pixels below its bottom border. As a temporary fix we introduce an argument need_to_invert_color into our display function. As of this writing 3 Jan 2019 we are just using it from here. The correct fix would be for the widget to stop leaking instead, and when that is done white text is unnecessary - and we might not notice. So we start by testing whether the area we will be overlaid over is indeed the wrong color. ETC. In other words a quick hack for a corner case, that doesn't fix the underlying bug (workaround) and even as a hack makes use of something that doesn't exist (a theme system), instead adding and documenting a half-assed thing tacked on.
- quietbritishjim 8y ago
  Of course this is just my opinion, but in many situations I think tacking one small change on to the easiest place is fine. My comment earlier wasn't meant as a criticism of that practice. It's just I would consider refactoring after this happens multiple times: at some point, all those little tweaks can add up enough that the original design of the "core" code gets lost in the noise. When exactly is the right time can be hard to figure out. Especially because, if enough hacks have accumulated that code really does need a reshuffle, then the job of refactoring is harder, which actually increases the temptation to put it off. But I wouldn't make a big sweeping change just because of one small hack that's a bit ugly.
  - logicallee 8y ago
    Thanks, this answers my question. I had always thought documenting half-assed solutions was particularly important not just for anyone looking at the code (to understand the whole code quickly, if they're new to it, make changes, etc - or even if the author comes back 6 months later, to be reminded of the assumptions that led to the hacks) but because whoever finally sits down to architect the rewrite will have some indications of the "hacks" (degrees of freedom) that must be made possible in the proper version. I thought they (or I) would sit down, have all the documented hacks on one side of their table, then use them to architect a nice solution that the hacks can then use instead, kind of like a requirements document. This was my thinking on a lot of projects. But you know what? That rewrite never became necessary! So not only did I not start with the "right" architecture - I didn't end with it either! That's what "many small hacks in the easiest of places" reminded me of and I wondered if you in fact do approve of it. It has always seemed fine for me. Just no problem at all. But with documentation right there and the worse the hack, the clearer the documentation right there explaining and justifying it, up to and including "I don't know why this works but this system call makes the next line succeed, whereas removing it causes the next line to fail sometimes - this is tested in testxzy." Obviously a hack, a terrible hack if you don't know why it works. And a project can end up with a lot of these.
lugg 8y ago
You're talking about rewrites. I'm talking about cleaning up messes left by lazy people long ago, so long ago that most of the juniors are scared to clean it up despite it being impossible to understand without a days reading. You can say it disrupts others understanding of shitty old code, frankly I don't care. Code is not immutable and maintaining status quo helps nobody except the old guard stay relevant.
- quietbritishjim 8y ago
  I said: > Of course, sometimes rewrites/refactors really are an improvement If that's what you were talking about in the first place, fair enough and my apologies.
  - lugg 8y ago
    No apologies needed. It was a fair assumption and I do agree with your points, although I think I've come to move more in the other direction the last few years. Those perfect one line fixes are great but they approach their limits and eventually need to be refactored. This is natural and should not be frowned upon. Likewise you can't just refactor all the time, one line fixes are faster and less costly in all sorts of ways. The key is to pick and choose when to use each strategy. I think we're too often looking for simple rules. There are none. Just a bunch of guidelines.

z0r 8y ago

Writing code is hard, so there's nothing wrong with reading it being hard too. (I'm only half kidding).

zorked 8y ago

Five people understand the system. You refactor it. Now one person understands the system.

Aeolun 8y ago
Initially the system took 8 hours of reading and 2 days of refactoring to grasp. Now it takes 2 hours. I’ll take that.

chucksmash 8y ago

Working Effectively with Legacy Code recommends just this idea. They call it scratch refactoring. Refactor without a lot of forethought to see how the system works, then just throw away your changes but keep your newfound understanding.

lugg 8y ago
This is effectively "it." But I do like to branch off the more useful refactors into their own commits.

pault 8y ago

I often see some bit of code, or a dependency in a project that I think is over engineered and too complex and start rewriting; first I get something basic working, then I discover some edge case, then another, and another, and eventually I realize that I have reimplemented the original code. It always makes me feel silly, but at the same time those have been the best learning experiences I've ever had.

zozbot123 8y ago
The proper response to that is to actively document these edge cases (within the original code) so that they don't get missed as the code is maintained in the future.
- pault 8y ago
  I do, so I can remember how it works when I see it again in six months. :)

titzer 8y ago

Take a moment to consider that someone else in your team might actually understand the code in question, and when you rewrite it (and commit), you destroy their knowledge about how it works. Sure, from your perspective, you made the code better, easier to understand. From their perspective, even if they do the code review themselves, they will not understand it as thoroughly as you do, and they will be burdened with both the knowledge of how it used to be, and how it is now. The net result can be negative, and eventually no one understands any code that they didn't refactor in the last ~6 months, because if it's any older than that, someone's rewritten it. Sometimes you gotta sit down and bite the bullet and read and ask questions.