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

121–130 of 144 posts

Re: How I review code

#121

Post by Bjartr »

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.

Making foolish mistakes aka learning?

Re: How I review code

#122

Post by bluesnowmonkey »

pmcollins wrote: post #106
athenot wrote: post #70

Earlier quoted context omitted.

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

People don't just move between teams, they also move between companies, even moreso in my experience.

There's this mindset in some companies that they're IBM, they are the world, but the it's not like that anymore. You can't expect people to sit through your company training on how to use your proprietary tools and then spend 30 years working there. You want to be able to use all types of people coming in with all types of experience because it's very diverse out there. And they want to use and build skills that they can take to their next company or leverage to start their own company. You want highly motivated individuals with entrepreneurial mindsets right? What kind of environment will attract them?

Go ahead and say, "We only use COBOL here," and see how that works out. Great everything is interoperable. But you've got a team of C players. Maybe that's fine though.

Re: How I review code

#123

Post by logfromblammo »

Stratoscope wrote: post #19
xerophyte12932 wrote: post #17

Earlier quoted context omitted.

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

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…

That isn't actually important unless you have multiple algorithms, which is when you create an ISquareRootApproximator interface, and have NewtonRaphsonAlgorithm and any other classes implement it.

Then you can have them duke it out running identical unit tests in the profiler.

That creates a clean separation between the people who just need to know what a function does and those that need to know how that function works. People who just need an approximated square root fast can understand perfectly well what ISquareRootApproximator.ApproximateSquareRoot( r ) does, and don't necessarily care whether your "get me a square root approximator" function returns a NewtonRaphsonAlgorithm, or a CORDICAlgorithm, or a BitshiftsMagicNumbersAndLookupTablesAlgorithm, or something else, so long as it approximates a square root.

Re: How I review code

#124

Post by lilactown »

pmcollins wrote: post #106
athenot wrote: post #70

Earlier quoted context omitted.

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

IMO, we should focus more on standardizing on runtimes rather than language. Languages evolve, and new ones come out that solve problems better. We luckily live in an age where many of those languages can interop at the runtime level.

As a developer, and as a team, it sucks to be told "you must use Java and JavaScript for time immemorial." An easy compromise is that any package/library/service/etc. must target the JVM or JavaScript VM and provide interop with existing code.

Re: How I review code

#125

Post by macobo »

dhimes wrote: post #101
macobo wrote: post #23

Earlier quoted context omitted.

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.

How so? The code still documents what is happening, the commits however lay out the whys.

The point is that these two are separate questions, and that trying to use comments as a crutch to join the two religiously is a headache. It's impossible to keep everything in sync and I don't want to read needless or worse misleading information.

What's worse, in comments we often omit the important details such as why was the change made, what other choices were considered, how was the thing benchmarked, etcetc.

That said, comments still have a place. Just not everywhere for everything and especially not for documenting history.

Re: How I review code

#126

Post by erroneousfunk »

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.

> they've picked the right abstraction for the problem

I hate to agree with you, but "picking the right abstraction for the problem" elegantly expresses what I've been trying to tell people for the last few years now, but far less succinctly (I usually ramble on about "underlying data models" and "what this actually is in the real world, not just how we view it in our application")

The reason I hate to agree with you is that I just became very disappointed that this is a difficult skill to master. "Just use the right model and the code is easy, duh. Why aren't you doing this?" Well. Now I just feel like a jerk. I thought they just gave me the "senior" title because I was old.

It's unfortunate that one of the most important skills in the industry is so intangible and difficult to quantify. Even more difficult to teach.

Re: How I review code

#127

Post by erroneousfunk »

pencilhappen wrote: post #87
athenot wrote: post #70

Earlier quoted context omitted.

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…

> 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). Not quite true... not all languages have a sweet spot in a production environment. Node.js isn't particularly excellent at anything, the attraction is mainly "I know…

I had a manager that loved Java. He wanted to add a new helper service onto an existing service that was written in Python, but the new service would be Java (because Java is the best, obviously).

This new helper was responsible for transforming, combining, and synthesizing large API responses with somewhat variable and highly nested data structures into other large API responses with somewhat variable and highly nested data structures. It certainly was not Java I was proud of, but it would have been trivial (and FAR more readable) in Python. Ugh.

Re: How I review code

#128

Post by athenot »

pencilhappen wrote: post #87
athenot wrote: post #70

Earlier quoted context omitted.

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…

> 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). Not quite true... not all languages have a sweet spot in a production environment. Node.js isn't particularly excellent at anything, the attraction is mainly "I know…

> Node.js isn't particularly excellent at anything

That's not very kind. Yes it has its flaws but it's not bad at being a higher abstraction over async-everything, augmented with a huge module repository. Support for isomorphic javascript can come in handy for some applications.

I've never done Scala but my understanding of it is that it could be a good fit for modelling certain types of business logic while being able to take advantage of exiting java libs out there.

> Knowing more than one tool in the toolbox is a key skill, since there is no one-size-fits-all.

Yes, we agree here. That was the point I was making.

Re: How I review code

#129

Post by dhimes »

macobo wrote: post #125
dhimes wrote: post #101

Earlier quoted context omitted.

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

How so? The code still documents what is happening, the commits however lay out the whys. The point is that these two are separate questions, and that trying to use comments as a crutch to join the two religiously is a headache. It's impossible to keep everything in sync and I don't want to read needless or worse misleading information. What's worse, in comments we often omit the important details such as why was the…

I disagree. I think the "whys" belong in the comments- in fact, that's the most important part of the comment if the code is cleanly written. I don't want to be happily coding along, get to a glob and have to go to the repo pane, hunt for the commit that explains this particular thing, then read a commit message. Put it in a comment in the code. Pretty please.

Re: How I review code

#130

Post by news_to_me »

cobbzilla wrote: post #119
news_to_me wrote: post #110

Earlier quoted context omitted.

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

sure but wouldn't you agree that being in business at all, and not unemployed, is a bigger thing, and that the luxury to play is earned by success. if you can't pay people including yourself, I suppose you can always stay at home and play/experiment all you want.

It depends on your perspective. Different people are motivated by different aspects of engineering – lots of folks simply like "playing" as you put it, and the business's bottom line isn't important (except insofar as the business can employ them).

From the business's perspective, they want the best engineers they can get, regardless of the engineer's motivations — as long as they can get them to provide enough value to the business, it can be worth some trade-offs like having hackweeks, etc. It's the business's job to align the incentives of their workers, and sometimes this includes throwing them a bone in choosing a preferred language, even though it may not be the most efficient.

Post reply on HN