4 ms·

> DO: Assume competence. The code reviews I've had the most trouble with are the ones where I can't. Blatant copy/pasting, 20 blank lines checked in, unrelated

by pkamb 7y ago

> DO: Assume competence. The code reviews I've had the most trouble with are the ones where I can't. Blatant copy/pasting, 20 blank lines checked in, unrelated configuration changes included, one-line fixes implemented via complex logic, zero git hygiene/cleanup, zero self-review of the code review before assigning it... If I leave a comment like: > This logic is repeated in both view controllers. Should be DRY. I'd love to be in a situation where that was actually intentional and a DRY implementation was considered but abandoned for X reasons. Far more likely is that they needed it in 3 places and thus pressed CMD-V 3 times.

Aeolun 7y ago
I think I can live with all of those things, if only the code actually works after it is merged. But too often you merge the crap and it doesn’t even work... That just baffles me, you’ve been writing all this code, but you never bothered to test that it actually did something?
BeetleB 7y ago
> This logic is repeated in both view controllers. Should be DRY. It may well be a good idea not to be DRY. https://news.ycombinator.com/item?id=20395901 https://news.ycombinator.com/item?id=20395901
- pkamb 7y ago
  > I'd love to be in a situation where that was actually intentional and a DRY implementation was considered but abandoned for X reasons.
smileysteve 7y ago
These might speak some to hiring, training, tooling, and team culture. First, one of these (blank lines) seem to be self correcting with a code style linter (not linting creates PR noise, increases and reviewer and coding frustration). Second, blatant copy pasting should be difficult, as surely the context is different - thus descriptive names should often be renamed, and then at least some level of integration testing provides quick education to dry up code. And surely, if the team is reviewing, calling out repeated or obviously copied untested code, with no linting the developer would improve rapidly otherwise tire of the rework.

by pkamb 7y ago

Aeolun 7y ago

I think I can live with all of those things, if only the code actually works after it is merged. But too often you merge the crap and it doesn’t even work... That just baffles me, you’ve been writing all this code, but you never bothered to test that it actually did something?

BeetleB 7y ago

> This logic is repeated in both view controllers. Should be DRY. It may well be a good idea not to be DRY. https://news.ycombinator.com/item?id=20395901 https://news.ycombinator.com/item?id=20395901

pkamb 7y ago
> I'd love to be in a situation where that was actually intentional and a DRY implementation was considered but abandoned for X reasons.

smileysteve 7y ago

These might speak some to hiring, training, tooling, and team culture. First, one of these (blank lines) seem to be self correcting with a code style linter (not linting creates PR noise, increases and reviewer and coding frustration). Second, blatant copy pasting should be difficult, as surely the context is different - thus descriptive names should often be renamed, and then at least some level of integration testing provides quick education to dry up code. And surely, if the team is reviewing, calling out repeated or obviously copied untested code, with no linting the developer would improve rapidly otherwise tire of the rework.