Hacker News new | ask | show | jobs
by RHSeeger 998 days ago
I trust and respect the people I work with. I also prefer to have code reviews, even (especially) of my own code. Code reviews can point out code that

- Doesn't consider an edge case (NPEs being the most common)

- Doesn't match up well with the "standard" way we do things (and, all things being equal, using the same approach in every place improves maintainability)

- Has a simpler approach

- Doesn't have a comment where it's not clear _why_ the code is doing something (that the author of the code sees as obvious, because they wrote it)

- Has misleading method/class/field names

These are all things that I've seen from both myself and others on my team. Having one other team member take a look at my code before it goes to QA can save a lot of time later.

1 comments

None of the things you said matters if your issue is time to market or you have little business.

Who cares about an edge case no user's gonna see?

Who cares about an edge case for a non business critical feature?

Who cares if there was a better way to implement something if it's not a priority?

Our work it's not meant to give us devs intellectual challenges but solve people's problems.

All the things you list are reasonable if you're making money or they are core to the product or they impact user's safety in any way.

Otherwise they don't matter at all.

And half the things you list are solved by having high hiring standards and paying people well.

Stuff may not be perfect, maybe there will be better ways, maybe it will be inconsistent at times but it will be more than good enough and move the business forward at much higher speed. Also, just to reiterate I've never stated that we don't have standards or make architectural decisions or don't give feedback. I merely stated that reviewing PRs for us is an exception, not a rule and that I prefer a system of trust.

My goal is to build things that work, and can continue to be worked on and improved over time; where development doesn't grind to a halt in 6 months because nobody cares about quality.

Edge cases matter sometimes matter and sometimes do not. They particularly matter in cases where, if the happen, the leave the system in a bad state (corrupt data, etc). Having a second pair of eyes take a look at the code and think "where can this go wrong" doesn't cost much, but can save a lot of pain later.

Doing things in a simpler way and/or a way that matches the way we generally do things makes it easier to maintain. And it's fairly common for code maintenance and/or further development on the same code, to be more common than writing brand new functionality (code that doesn't touch existing code). This is directly related to the "grind to a halt" I mentioned earlier.

Literally nothing I mentioned can be solved by hiring standards or pay. Once, because hiring people new to software development is a thing. Two, because everyone... EVERYONE makes mistakes. And EVERYONE can do better with the help of their peers.

I prefer that every PR get reviewed by someone, with the (uncommon) exception of those where the developer says it isn't worth a review. It generally takes very little time, and it adds to delivered code quality.