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

71–80 of 144 posts

Re: How I review code

#71

Post by ryanianian »

nimbix wrote: post #43

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.

This is an ideal but is hardly scalable if you're doing 3-4+ PRs per day and are expected to do your own coding as well (plus attend bureaucracy).

You can effectively do this by checking that every nontrivial change has sufficient automated test coverage. This saves you from having to test changes yourself and saves future devs from having to go through your thought-process when they touch that code next.

Re: How I review code

#72

Post by cobbzilla »

dboreham wrote: post #52
pmcollins wrote: post #44

> 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…

Well there's a can of worms! Thoughts based on experience: Asymmetric information situation: newly hired smart engineer says that new subsystem should be written in . He says it is so much better than . Everyone on HN says it is so much better. You however, haven't had the time to try it out on a medium sized project to determine if this is true or the usual new language hype. New Engineer seems to know plenty about…

The answer to both situations is "No" and "No". Many engineering managers have a difficult time saying "no", to the detriment of the business.

We are building a product for customers, not a playground or post-graduate program. There are legitimate reasons to add another language but they must be evaluated with the needs of the business in mind, these include long-term maintenance costs and hiring/training costs regarding new/esoteric skill sets, among others.

Re: How I review code

#73

Post by MaxBarraclough »

benji-york wrote: post #51
taneq wrote: post #33

Earlier quoted context omitted.

> Occasionally a reviewer will spot a bug. Occasionally there will be a false positive that turns out not to be a bug. And most often of all, while explaining what they've done, the reviewee will say "and this does... oh crap, wait" and fix the mistake themselves. Among all their other virtues, a code reviewer is a level 2 debugging duck. (Although all of my code review experience is with buddy check-in style reviewi…

This "oh crap" effect is one of the reasons I like "self review" as the first step in a code review process. That is, go over the diff and add commentary explaining to the reviewer why you made particular changes (not explaining what the changes do, that should be in comments in the source).

GitHub supports posting comments to a commit. Might be a good fit.

Re: How I review code

#74

Post by testouts »

dboreham wrote: post #47
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.

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

ding ding ding

Re: How I review code

#75

Post by cobbzilla »

darkerside wrote: post #64
yeukhon wrote: post #2

I echo the author's point in "Review the code with its author in mind". Without comments, sometimes it's really really difficult to navigate the code. I have been adding more comments than ever: don't assume every line is obvious, write a comment to explain what the next few lines really do. # base case: stop dividing when we find the largest square. if width == height: return width, height else: # otherwise, we know…

> I echo the author's point in "Review the code with its author in mind". I don't understand how the rest of your post relates to that, although I think it's an interesting point and following discussion. On the topic of reviewing code with the author in mind, I'm not sure I agree at all with the linked article. Does it matter who wrote the code in any way? Good code is good, and bad code is bad. It may be a helpful…

If you know your team well, it will help you keep an eye out for common mistakes they've made in the past. It may also help adjust your tone, as developers you've worked with for a long time will understand light humor or other well-intended comments that might be read as off-putting by newer devs.

Re: How I review code

#76

Post by bluntfang »

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.

is this a no true scottsman?

Re: How I review code

#77

Post by virmundi »

Sir_Cmpwn wrote: post #54
pmcollins wrote: post #44

> 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…

Why?

The more languages you have the more you have to know just to know what the software knows. This makes things hard. Heck jumping from what I call back end languages like Go to Java to C# requires a mental switch if you've been doing one for a week or more. I'm using Go now, but I've got 13 years of Java experience with a Java Ring (https://images.techhive.com/images/idge/imported/article/jvw...) on my hand. When I switch back to Java I mutter the phrase, "Nope. Nope. Nope. That's Go, not Java."

You should use the right tool for the job. Python is good for AI stuff. Java is good for performance and maintainability. Go is interesting any might give Java a run for its money.

You should limit adding tools because they're cool. Do that for bow ties not code.

Re: How I review code

#78

Post by megous »

stctgion wrote: post #65
pmcollins wrote: post #44

> 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…

Totally agree with this. I've had to pick up the pieces on more than one occasion when some key piece of code was written in the flavour of the month and the writer shortly left. I've heard other Devs say you should pick the right language for the job but I think there should be a heavy bias to what the rest of the code base already is using

None of those, except perhaps for the golang are a flavor of the month languages. Using "Right tool for the job" is also important.

Re: How I review code

#79

Post by volkk »

cobbzilla wrote: post #72
dboreham wrote: post #52

Earlier quoted context omitted.

Well there's a can of worms! Thoughts based on experience: Asymmetric information situation: newly hired smart engineer says that new subsystem should be written in . He says it is so much better than . Everyone on HN says it is so much better. You however, haven't had the time to try it out on a medium sized project to determine if this is true or the usual new language hype. New Engineer seems to know plenty about…

The answer to both situations is "No" and "No". Many engineering managers have a difficult time saying "no", to the detriment of the business. We are building a product for customers, not a playground or post-graduate program. There are legitimate reasons to add another language but they must be evaluated with the needs of the business in mind, these include long-term maintenance costs and hiring/training costs regar…

Surely there's a middle ground? Plenty of good companies allow for fun experimentation. However, I also wouldn't say that switching to a new language or frame work that may boost long term productivity and hiring effectiveness is considered a playground

Re: How I review code

#80

Post by Sir_Cmpwn »

virmundi wrote: post #77
Sir_Cmpwn wrote: post #54

Earlier quoted context omitted.

Why?

The more languages you have the more you have to know just to know what the software knows. This makes things hard. Heck jumping from what I call back end languages like Go to Java to C# requires a mental switch if you've been doing one for a week or more. I'm using Go now, but I've got 13 years of Java experience with a Java Ring ( https://images.techhive.com/images/idge/imported/article/jvw... ) on my hand. When I…

Big enough companies may have lots of different projects and many individuals won't work on more than two or three languages max on the daily, though.
Post reply on HN