6 ms·

Feels like this could be a case where DRY could fix a bug in one place

by pacaro 7y ago

kichik 7y ago
I was thinking the same so I checked and they removed the duplication: https://github.com/php/php-src/commit/3d9c02364db62a6d8e27947ffe47dbfaad644efe#diff-e0dff85f21e939e4e2a778bddb8a72d7 https://github.com/php/php-src/commit/3d9c02364db62a6d8e2794... They also fixed the whitespace handling that let something like "RandomHeader: hello host:8080" mistakenly set the flag. https://github.com/php/php-src/commit/56cdbe63c24b86c2f1d60bf2609fde113d12d235#diff-e0dff85f21e939e4e2a778bddb8a72d7 https://github.com/php/php-src/commit/56cdbe63c24b86c2f1d60b...
- pacaro 7y ago
  That looks so much better to me. The cognitive load is significantly reduced
sjwright 7y ago
DRY is a good principle, but sometimes I’d rather repeat myself a little bit than build more bug-prone scaffolding to avoid repetition—especially when the repetition isn’t line-for-line identical. Just remember to always cite the repetition in comments.
- fiddlerwoaroof 7y ago
  I’ve never come across a bug due to application of DRY, but I have seen many bugs because of “harmless” duplication resulting in inconsistent code changes several months later. Even aside from DRY, extracting a block of code to a function or method gives you an opportunity to name the block of code, which often significantly clarifies the intent of the code so, I’ve always tried to error on the side of overly DRY
  - smichel17 7y ago
    I haven't seen bugs due to DRY, but I've caused difficulty of adding new features and poor maintainability because of it. When I first took over maintaining Red Moon, I was a very new dev and went a little DRY crazy. In particular, there's a state machine for the different filter states (running, paused, stopped, etc), that had a bunch of classes (one per state) that had a lot of overlap. I pulled some common behaviors out into a supertype that the classes with those behaviors now inherited from. Except, it turns out some of those states ought to act differently (automatic pauses in secure apps should disable the overlay but not restore the backlight; manual pauses should do both), and now it's going to be extra work untangling the various states and pause logic. If I'd left the state machine (amongst other bits) as it was, this feature/bugfix would be implemented already. Also, the current state of things (pun intended) is a bit less readable, in my opinion.
    - fiddlerwoaroof 7y ago
      I’ve never really understood this, because undoing DRY is relatively easy: you either copy the new function/class and rename it or you inline it in the mistaken case and adjust the code to match.
      - smichel17 7y ago
        Easy, but it's still work. As a thought experiment: if it's so easy, would you be interested in doing it for me? (edit: Obviously not, but compare your internal resistance to if I'd asked you to do something trivial like fix a typo in the readme) Here's the file I was referencing (only 100 lines!): https://github.com/LibreShift/red-moon/blob/master/app/src/main/java/com/jmstudios/redmoon/filter/Command.kt https://github.com/LibreShift/red-moon/blob/master/app/src/m... Here's the issue I've been putting off fixing because of it: https://github.com/LibreShift/red-moon/issues/208 https://github.com/LibreShift/red-moon/issues/208 The problem is that right now there is one PAUSE state, which is triggered by both manual pauses (brightness should be restored) and automatic pauses (in secure apps; brightness should not be restored). The problem wasn't exactly DRY, it's the misapplication: I merged bits that had identical code, but turned out not to have identical purpose. And, for posterity, here's what it used to look like way back (DRYing was not the only change, I also split out notification stuff, so it's not a 1:1 length comparison): https://github.com/LibreShift/red-moon/blob/11ae916955ff8c360eb568c3b5632673b50b58d0/app/src/main/java/com/jmstudios/redmoon/presenter/ScreenFilterPresenter.kt https://github.com/LibreShift/red-moon/blob/11ae916955ff8c36... edit: and here's the original file, back mostly before I touched it at all: https://github.com/LibreShift/red-moon/blob/ed2ec4fd1c68611d851a55f3ade141e6738e394c/app/src/main/java/com/jmstudios/redmoon/presenter/ScreenFilterPresenter.java https://github.com/LibreShift/red-moon/blob/ed2ec4fd1c68611d...
  - ivanhoe 7y ago
    I've seen people over-engineering a piece of code in order to make it "flexible" enough to be able to reuse it in like 2-3 scenarios that are not really well aligned - all in the name of making their code super DRY. Sometimes it's better to have just a few separate, differently shaped pegs, than one flexible peg that fits any hole but is so complex that it might introduce 10 new bugs along the way.
  - ljm 7y ago
    I’ve seen plenty of DRY bugs due to premature abstraction and an unwillingness to refactor later on. In many cases, the opportunity for DRY was misleading as the code was only superficially similar, so the implementation became over-complicated to handle various corner cases that otherwise wouldn’t have existed.
- namibj 7y ago
  Last time this caused memory corruption, I did a rewrite using eacaping-m4 to encode this difference in behavior. Gnu m4 has a changeword feature you can compile in for that.

Feels like this could be a case where DRY could fix a bug in one place

by pacaro 7y ago

Feels like this could be a case where DRY could fix a bug in one place

I was thinking the same so I checked and they removed the duplication: https://github.com/php/php-src/commit/3d9c02364db62a6d8e27947ffe47dbfaad644efe#diff-e0dff85f21e939e4e2a778bddb8a72d7 https://github.com/php/php-src/commit/3d9c02364db62a6d8e2794... They also fixed the whitespace handling that let something like "RandomHeader: hello host:8080" mistakenly set the flag. https://github.com/php/php-src/commit/56cdbe63c24b86c2f1d60bf2609fde113d12d235#diff-e0dff85f21e939e4e2a778bddb8a72d7 https://github.com/php/php-src/commit/56cdbe63c24b86c2f1d60b...

pacaro 7y ago
That looks so much better to me. The cognitive load is significantly reduced

sjwright 7y ago

DRY is a good principle, but sometimes I’d rather repeat myself a little bit than build more bug-prone scaffolding to avoid repetition—especially when the repetition isn’t line-for-line identical. Just remember to always cite the repetition in comments.

fiddlerwoaroof 7y ago
I’ve never come across a bug due to application of DRY, but I have seen many bugs because of “harmless” duplication resulting in inconsistent code changes several months later. Even aside from DRY, extracting a block of code to a function or method gives you an opportunity to name the block of code, which often significantly clarifies the intent of the code so, I’ve always tried to error on the side of overly DRY
- smichel17 7y ago
  I haven't seen bugs due to DRY, but I've caused difficulty of adding new features and poor maintainability because of it. When I first took over maintaining Red Moon, I was a very new dev and went a little DRY crazy. In particular, there's a state machine for the different filter states (running, paused, stopped, etc), that had a bunch of classes (one per state) that had a lot of overlap. I pulled some common behaviors out into a supertype that the classes with those behaviors now inherited from. Except, it turns out some of those states ought to act differently (automatic pauses in secure apps should disable the overlay but not restore the backlight; manual pauses should do both), and now it's going to be extra work untangling the various states and pause logic. If I'd left the state machine (amongst other bits) as it was, this feature/bugfix would be implemented already. Also, the current state of things (pun intended) is a bit less readable, in my opinion.
  - fiddlerwoaroof 7y ago
    I’ve never really understood this, because undoing DRY is relatively easy: you either copy the new function/class and rename it or you inline it in the mistaken case and adjust the code to match.
    - smichel17 7y ago
      Easy, but it's still work. As a thought experiment: if it's so easy, would you be interested in doing it for me? (edit: Obviously not, but compare your internal resistance to if I'd asked you to do something trivial like fix a typo in the readme) Here's the file I was referencing (only 100 lines!): https://github.com/LibreShift/red-moon/blob/master/app/src/main/java/com/jmstudios/redmoon/filter/Command.kt https://github.com/LibreShift/red-moon/blob/master/app/src/m... Here's the issue I've been putting off fixing because of it: https://github.com/LibreShift/red-moon/issues/208 https://github.com/LibreShift/red-moon/issues/208 The problem is that right now there is one PAUSE state, which is triggered by both manual pauses (brightness should be restored) and automatic pauses (in secure apps; brightness should not be restored). The problem wasn't exactly DRY, it's the misapplication: I merged bits that had identical code, but turned out not to have identical purpose. And, for posterity, here's what it used to look like way back (DRYing was not the only change, I also split out notification stuff, so it's not a 1:1 length comparison): https://github.com/LibreShift/red-moon/blob/11ae916955ff8c360eb568c3b5632673b50b58d0/app/src/main/java/com/jmstudios/redmoon/presenter/ScreenFilterPresenter.kt https://github.com/LibreShift/red-moon/blob/11ae916955ff8c36... edit: and here's the original file, back mostly before I touched it at all: https://github.com/LibreShift/red-moon/blob/ed2ec4fd1c68611d851a55f3ade141e6738e394c/app/src/main/java/com/jmstudios/redmoon/presenter/ScreenFilterPresenter.java https://github.com/LibreShift/red-moon/blob/ed2ec4fd1c68611d...
- ivanhoe 7y ago
  I've seen people over-engineering a piece of code in order to make it "flexible" enough to be able to reuse it in like 2-3 scenarios that are not really well aligned - all in the name of making their code super DRY. Sometimes it's better to have just a few separate, differently shaped pegs, than one flexible peg that fits any hole but is so complex that it might introduce 10 new bugs along the way.
- ljm 7y ago
  I’ve seen plenty of DRY bugs due to premature abstraction and an unwillingness to refactor later on. In many cases, the opportunity for DRY was misleading as the code was only superficially similar, so the implementation became over-complicated to handle various corner cases that otherwise wouldn’t have existed.
namibj 7y ago
Last time this caused memory corruption, I did a rewrite using eacaping-m4 to encode this difference in behavior. Gnu m4 has a changeword feature you can compile in for that.