No Limits on Repetition – The Daily WTF

Date:

Share:

Just because you get fired doesn’t mean that your pull requests are automatically closed. Dallin was in the middle of reviewing a PR by Steve when the email came out announcing that Steve no longer worked at the company.

Let’s take a look at that PR, and maybe we can see why.

                    $originalUndrawn = DecimalHelper::toDecimal($party->limit)->sub(DecimalHelper::toDecimal($party->drawn));

This is the original code, which represents operations on investments. An investment is represented by a note, and belongs to one or more partys. The amount that can be drawn is set by a limit, which can belong to either the party or the note.

What our developer was tasked with doing was allow a note to have no limit. This means changing all the places where the note‘s limit is checked. So this is what they submitted:

                    if ($note->limit == null) {
                        $originalUndrawn = DecimalHelper::toDecimal($party->limit)->sub(DecimalHelper::toDecimal($party->drawn));
                    } else {
                        $originalUndrawn = DecimalHelper::toDecimal($party->limit)->sub(DecimalHelper::toDecimal($party->drawn));
                    }

You’ll note here that the note limit isn’t part of calculating the party limits, so both branches do the same thing. And then there’s the deeper question of “is a null really the best way to represent this?” especially given that elsewhere in the code they have an “unlimited” flag that disables limit checking.

Now, Steve wasn’t let go only for their code- they were just a miserable co-worker who liked to pick fights in pull request comments. So the real highlight of Steve’s dismissal was that Dallin got to have a meaningful discussion about the best way to make this change with the rest of the team, and Steve didn’t have a chance to disrupt it.

[Advertisement]
Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.

Source link

Subscribe to our magazine

━ more like this

Intercom on Product: Designing for Clarity

As Intercom’s product scaled, we struggled to keep the navigation clear and user-friendly. So how do you redesign information architecture without disrupting users? Senior Product...

FBI, Justice Department Buildings Briefly Listed For Sale

The Trump administration briefly listed over 440 federal buildings for sale online before suddenly removing the document, which included major properties like the FBI...

Why OPI is My Favorite Nail Polish

If you've been a long-time reader of this blog you'll know how I have an especially warm spot in my heart for nail polish....

I Tried Prime Video’s New AI Dubbing, and I Have Thoughts

We may earn a commission from links on this page. ...