I found and fixed a bug in PHP's standard library • Hacker News Board

Re: I found and fixed a bug in PHP's standard library

#31

Post by dkneeland » Tue, Mar 03, 2020, 2:57 AM UTC

smichel17 wrote: post #13

fiddlerwoaroof wrote: post #12

Earlier quoted context omitted.

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

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 behavio…

Thanks for maintaining the app btw! Makes it a lot easier to get to sleep after staring at HN for an hour before bed.

Re: I found and fixed a bug in PHP's standard library

#32

Post by pacaro » Tue, Mar 03, 2020, 7:17 PM UTC

kichik wrote: post #6

pacaro wrote: post #3

Earlier quoted context omitted.

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/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/56cdbe63c24b86c2f1d60b...

That looks so much better to me. The cognitive load is significantly reduced

Re: I found and fixed a bug in PHP's standard library

#33

Post by smichel17 » Thu, Mar 12, 2020, 12:32 AM UTC

fiddlerwoaroof wrote: post #14

smichel17 wrote: post #13

Earlier quoted context omitted.

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 behavio…

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.

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/m...

Here's the issue I've been putting off fixing because of it: 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/11ae916955ff8c36...

edit: and here's the original file, back mostly before I touched it at all: https://github.com/LibreShift/red-moon/blob/ed2ec4fd1c68611d...