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

101–110 of 144 posts

Re: How I review code

#101

Post by dhimes »

macobo wrote: post #23
Stratoscope wrote: post #19

Earlier quoted context omitted.

That article doesn't make a compelling case. It suggests taking this code: // square root of n with Newton-Raphson approximation r = n / 2; while ( abs( r - (n/r) ) > t ) { r = 0.5 * ( r + (n/r) ); } System.out.println( "r = " + r ); And refactoring it to this function: private double SquareRootApproximation(n) { r = n / 2; while ( abs( r - (n/r) ) > t ) { r = 0.5 * ( r + (n/r) ); } return r; } System.out.println( "r…

There's an infinite amount of detail that's impossible to capture in a comment and which invariably changes over time and doesn't hold in the future. For my team, the solution has been writing longer commit messages detailing not only what has changed, but also the why and other considerations, potential pitfalls and so forth. So in this case, a good commit message might read like: ``` Created square root approximati…

Now you have to look in two places for the information- the code and the commit messages.

Re: How I review code

#102

Post by 52-6F-62 »

ot wrote: post #60
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.

Reminds me of the "Evolution of a Haskell programmer": https://www.willamette.edu/~fruehr/haskell/evolution.html Make sure you don't miss the punchline, "Tenured professor". It's the same with the progression of engineering seniority: increasing levels of cleverness and unnecessary sophistication, until you reach a point where you don't have anything to prove anymore, and you can feel comfortable writing the simplest…

That was great— though not being versed in Haskell, I tried to follow their link to the original version, but it had died.

Here's the original:

http://www.ariel.com.au/jokes/The_Evolution_of_a_Programmer....

The punchline got me pretty good.

Re: How I review code

#103

Post by Spivak »

xerophyte12932 wrote: post #17
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…

A good counter argument to Comments: https://blog.codinghorror.com/coding-without-comments/

I think his example proves the opposite of what he intends. His example is just begging for a discussion of how the approximation actually works, edge case considerations, error bounds, assumptions, limitations, complexity, and references. At least that's what I would want if I was looking at it with fresh eyes. Sure, the author's contact info is a little tongue-in-cheek but without comments I can only know what the code does, not what it's supposed to do.

Re: How I review code

#104

Post by spathi_fwiffo »

ams6110 wrote: post #83
MaxBarraclough wrote: post #25

Earlier quoted context omitted.

Disagree. A code-review is not purely checking for non-functional properties of the code, it's checking for overall code quality, and that includes bugs. Occasionally a reviewer will spot a bug. Occasionally there will be a false positive that turns out not to be a bug. You really want to deliberately discard this bug-finding opportunity? Why? Even if it's a false positive, doesn't that indicate that something bears…

> Occasionally a reviewer will spot a bug Isn't the code review generally pretty late, i.e. just prior to release? At that point, the code should be passing all unit tests and I'd expect obvious bugs to be pretty unlikely. Non-obvious bugs generally won't be spotted in a code review setting.

Not sure what you mean by 'late'. I guess it all depends on process defined for your project. Most SCM systems have some way to make code visible before it is published to the live code base (via branches, etc).

Also, passing tests is meaningless if the tests are worthless or incomplete. So, those alone can't be used as a metric of code sanity; Meaning obvious bugs are still possible.

Tests and reviews are just to reduce the number of released bugs. and hopefully increase maintainability.

Also, what's obvious to one developer might not be to another.

Re: How I review code

#105

Post by virmundi »

Sir_Cmpwn wrote: post #89
virmundi wrote: post #85

Earlier quoted context omitted.

Give then context of the conversation I think the company was limited to one team or perhaps a few. Even within a larger company multiple languages limits the effectiveness of Human Resources. You can't as easily move people between teams.

The context? Tumblr? They have over 400 employees.

That doesn't matter. I've worked in large insurance companies that had SQL procs, Java, and Old Cobol. They had over 1k IT employees across the country. Sure there are odds and ends in bash, but bulk of the core business systems were in Java.

Re: How I review code

#106

Post by pmcollins »

athenot wrote: post #70
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? Having many languages allows to pick the sweetspot for each subsystem. Each language has special strengths and weaknesses, there is no silver bullet that excels at everything. Go, C/C++, Lua, Ruby, Perl, Scala, Node.js, Python... each of these are THE best choice for certain classes of problems (and terrible for others). It may be because of language features that elegantly express a solution, or particular effi…

But there are so many benefits to standardizing on one language (to take the extreme case). You create the business rules, APIs, and utilities in whatever language you choose and that can be shared by everyone. That becomes the core of your business. If you need something new, you create it and submit it for review, and then everyone can use it, not just the folks who choose what was your preferred language at the time of writing.

I've been in organizations where you have the Language/Platform X people over here and the Language/Platform Y people over there, and they duplicate work, don't collaborate, and their libraries don't interoperate. They have different hiring needs, and developers can't easily move between them. Ultimately, everybody with their pet languages moves on, and new people come into a very fractured environment and ask themselves WTF is this? Over the years, developers (especially new ones) are only a fraction as effective as they would be if the company had just paid the relatively small up-front price of nudging everyone into the same ecosystem.

And BTW, I have yet to encounter a problem that is hard to solve in my preferred language, but is way easier to solve in another. I think the key to that is to choose carefully what you're going to commit to.

Re: How I review code

#107

Post by Consultant32452 »

athenot wrote: post #70
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? Having many languages allows to pick the sweetspot for each subsystem. Each language has special strengths and weaknesses, there is no silver bullet that excels at everything. Go, C/C++, Lua, Ruby, Perl, Scala, Node.js, Python... each of these are THE best choice for certain classes of problems (and terrible for others). It may be because of language features that elegantly express a solution, or particular effi…

>Though having a diverse ecosystem fosters an openess of mind, instead of enforcing "One Metaphore To Rule Them All". You find yourself borrowing efficient patterns from other environments, as they eventually start cross-pollinating.

I've had the opposite experience. Having a lot of different technology stacks increases the barrier to entry for anyone who might be interested in working on another part of the ecosystem. This results in increased siloing of people and information.

Re: How I review code

#108

Post by lazyasciiart »

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.

I think of it more as a 'no true senior engineer' argument.

Re: How I review code

#109

Post by agentgt »

> clever code is often difficult to read and understand later

I have seen this many times and they are actually usually talented developers that are just not used to working in groups....

but what I have seen more often (back when I did code reviews)... is lazy copy and pasting or something analogous.

Re: How I review code

#110

Post by news_to_me »

cobbzilla wrote: post #86
volkk wrote: post #79

Earlier quoted context omitted.

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

If I read "good companies" to mean those that have enough excess resources to make lots of foolish mistakes, then yes, totally agree. "Keeping devs happy" by itself is a terrible reason to introduce a new language. If your developers cannot find happiness by working together and building something great, you have bigger problems.

> If your developers cannot find happiness by working together and building something great

Just a friendly reminder that there are lots of different kinds of people who approach their work differently. Not being "business-driven" isn't necessarily a bad thing.

Post reply on HN