No Limits on Repetition – The Daily WTF

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.