Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

How I review code

engineering.tumblr.com

Post reply on HN
Print view View original

41–50 of 144 posts

Re: How I review code

#41

Post by lukevers »

watwut wrote: post #35
adrianN wrote: post #14

Earlier quoted context omitted.

I'm an overly pedantic reviewer because I feel the review is one of the few places where I can counteract the accumulation of technical debt. Once the less than ideal code is in the codebase it's there to stay. I also ask a lot of pretty dumb questions during a review because I want to make sure that my understanding of the requirements matches the understanding the other engineer had.

> Once the less than ideal code is in the codebase it's there to stay. That is not objective fact across projects. That means either the process or culture is bad.

Not exactly. In a lot of fast paced environments it’s rare to get a chance to rewrite something that functions properly just because it could be labeled as technical debt. It’s not about culture, it’s about moving on to the next thing. I code review hard to make sure we have the smallest amount of technical debt possible because it’s usually there to stay for a considerable amount of time.

Re: How I review code

#42

Post by weego »

scalesolved wrote: post #34
taylodl wrote: post #29

"Senior engineers sometimes need to be reminded that highly performant, abstract, or clever code is often difficult to read and understand later, which usually means asking them for more inline comments and documentation." Ha! That's not a senior engineer. Senior engineers write the most simple-looking code that just works. In every rainy day scenario imaginable. The clever code writers aren't there yet.

Ha it does make me think of this tweet https://twitter.com/KevlinHenney/status/381021802941906944 You nailed it really, senior engineers code is the most simple looking as generally they've picked the right abstraction for the problem.

Is this a senior engineers are literally superheroes meme I've missed?

Everyone is capable of making poor decisions and straight up logic errors. Senior devs sometimes more-so because we tend to get entrenched in a particular issue solo for longer periods.

Re: How I review code

#43

Post by nimbix »

For me the most important part of reviewing any nontrivial changes it actually check out the branch and test every change I see. This keeps a lot of issues from reaching the QA team and catches issues they could have missed since they don't actually go through the code.

Re: How I review code

#44

Post by pmcollins »

> We have repositories for the PHP backend, our database schemas, our iOS (Swift/Obj-C) and Android (Java/Kotlin) mobile apps, infrastructure projects written in Go, C/C++, Lua, Ruby, Perl, and many other projects written in Scala, Node.js, Python, and more

Why do organizations allow this? I realize that some platforms require their own languages (iOS, Android), but outside of that, just pick one or two and hold the line.

Re: How I review code

#45

Post by driusan »

soneca wrote: post #30

I am a junior developer and my latest feedback was that one of the main skills I should develop is to make better, more well-thought, critic and deep code reviews (including of PRs from more senior developers). Any tips on how to improve this? Would a checklist help? Have a clear process on what to review first?

Unless your current reviews are really superficial probably not. You can only review at your level of understanding, so if they're asking for more depth to your code reviews you probably need to develop a deeper understanding of the architecture and design of your codebase. A checklist would do the opposite of that in most cases.

Re: How I review code

#46

Post by adrianN »

watwut wrote: post #35
adrianN wrote: post #14

Earlier quoted context omitted.

I'm an overly pedantic reviewer because I feel the review is one of the few places where I can counteract the accumulation of technical debt. Once the less than ideal code is in the codebase it's there to stay. I also ask a lot of pretty dumb questions during a review because I want to make sure that my understanding of the requirements matches the understanding the other engineer had.

> Once the less than ideal code is in the codebase it's there to stay. That is not objective fact across projects. That means either the process or culture is bad.

I've never seen a project where you go back to fix something just because its implementation is not ideal. If it works it stays, because there is always something more important to do.

Re: How I review code

#47

Post by dboreham »

scalesolved wrote: post #34
taylodl wrote: post #29

"Senior engineers sometimes need to be reminded that highly performant, abstract, or clever code is often difficult to read and understand later, which usually means asking them for more inline comments and documentation." Ha! That's not a senior engineer. Senior engineers write the most simple-looking code that just works. In every rainy day scenario imaginable. The clever code writers aren't there yet.

Ha it does make me think of this tweet https://twitter.com/KevlinHenney/status/381021802941906944 You nailed it really, senior engineers code is the most simple looking as generally they've picked the right abstraction for the problem.

Senior Engineers are probably in meetings, not writing code..

Re: How I review code

#48

Post by wiredfool »

soneca wrote: post #30

I am a junior developer and my latest feedback was that one of the main skills I should develop is to make better, more well-thought, critic and deep code reviews (including of PRs from more senior developers). Any tips on how to improve this? Would a checklist help? Have a clear process on what to review first?

A checklist can help. (he says, and then does a mental one because there isn't one nearby).

What I look for is:

1) What's the problem being solved? Does this look like a reasonable approach? Is the code pythonic (Obv: for python)?

2) What edge cases are there? Does this handle the important ones? Does it punt properly on the less important ones?

3) Look for a short list of bug classes that have come up in the project before that have lead to emergency patches. E.g. Decrefing, Checking mallocs, any exec sorts of things. (This is a clear application for a checklist)

4) Are there tests/documentation/other required fixtures and stuff?

5) Does the code generally match the style of the project?

1000) Code formating and whitespace and line wrapping and all that bikeshedding stuff.

Feel free to short circuit anywhere once it becomes clear that there's more work required.

Re: How I review code

#49

Post by cousin_it »

taylodl wrote: post #29

"Senior engineers sometimes need to be reminded that highly performant, abstract, or clever code is often difficult to read and understand later, which usually means asking them for more inline comments and documentation." Ha! That's not a senior engineer. Senior engineers write the most simple-looking code that just works. In every rainy day scenario imaginable. The clever code writers aren't there yet.

Ha! That's good engineers. Senior engineers write an ambitious unusable framework, get promoted and move on to the next project. So the article's advice is ironically spot on.

Re: How I review code

#50

Post by scalesolved »

weego wrote: post #42
scalesolved wrote: post #34

Earlier quoted context omitted.

Ha it does make me think of this tweet https://twitter.com/KevlinHenney/status/381021802941906944 You nailed it really, senior engineers code is the most simple looking as generally they've picked the right abstraction for the problem.

Is this a senior engineers are literally superheroes meme I've missed? Everyone is capable of making poor decisions and straight up logic errors. Senior devs sometimes more-so because we tend to get entrenched in a particular issue solo for longer periods.

Not at all and it's now how I view senior engineers. True senior engineers though do tend to find the right abstractions more often than other engineers but it doesn't mean they can't make huge mistakes with bigger implications.

My thoughts are also on actual senior engineers, not 2 years of experience etc.

Post reply on HN