Live data from Hacker News

How I review code

engineering.tumblr.com

81–90 of 144 posts

Re: How I review code

#81
> I look for code that is well-documented (both inline and externally), and code that is clear rather than clever. I’d rather read ten lines of verbose-but-understandable code than someone’s ninja-tastic one-liner that involves four nested ternaries.

"Clear" and "clever" aren't in opposition, and likewise "verbose" and "understandable" aren't correlated.

I think this characterisation, and especially the example, shows a lowest-common-denominator straw man of "clever one-liners" which seems to miss the reason that some people like them. In particular, it seems to be bikeshedding about how to write branches. The author doesn't say what those "ten lines of verbose-but-understandable code" would be, but given the context I took it to mean "exactly the same solution, but written with intermediate variables or if/else blocks instead".

This seems like an analogous situation to https://wiki.haskell.org/Wadler's_Law where little thought is given to what the code means, more thought is given to how that meaning is encoded (e.g. ternaries vs branches) and religious crusades are dedicated to how those encodings are written down (tabs vs spaces, braces on same/new lines, etc.).

Note that even in this simple example there lurks a slightly more important issue which the author could have mentioned instead: nested ternaries involve boolean expressions; every boolean expression can be rewritten in a number of ways; some of those expressions are more clear and meaningful to a human than others. For example, `loggedIn && !isAdmin` seems pretty clear to me; playing around with truth tables, I found that `!(loggedIn -> isAdmin)` is apparently equivalent, but it seems rather cryptic to me. This is more obvious if intermediate variables are used, since they're easier to name if they're meaningful.

In any case, compressing code by encoding the same thing with different symbols doesn't make something "clever". It's a purely mechanical process which doesn't involve any insights into the domain.

To me, code is "clever" if it works by exploiting some non-obvious structure/pattern in the domain or system. For example, code which calculates a particular index/offset in a non-obvious way, based on knowledge about invariants in the data model. Another example would be using a language construct in a way which is unusual to a human, but has the perfect semantics for the desired behaviour (e.g. duff's device, exceptions for control flow, etc.).

Such "clever" code is often more terse than a "straightforward" alternative, but that's a side-effect of the "cleverness" (finding an existing thing which behaves like the thing we want) rather than the goal.

If the alternative to some "clever" code is "10 lines of verbose but understandable code" then it's probably not that clever; so it's probably a safe bet to go with the latter. The real issues with clever code are:

- Whether the pattern it relies on is robust or subject to change. Would it end up coupling components together, or complicate implementation changes in unrelated modules?

- How hard it is to understand. Even if it's non-obvious, can it be understood after a moment's pondering; or does it require working through a textbook and several research papers?

- Whether the insights it relies on are enlightening or incidental, i.e. the payoff gained from figuring it out. This is more important if it's harder to understand. Enlightening insights can change the way we understand the system/domain, which may have many benefits going forward. Incidental insights are one-off tricks that won't help us in the future.

- How difficult it would be to replace; or whether it's possible to replace at all.

This last point is what annoys me in naive "clever vs verbose" debates, and prompted this rant, since it's often assumed that the only difference is line count. To me, the best "clever" code isn't that which reduces its own line count; it's the code which removes problems entirely; i.e. where the alternative has caveats like "before calling, make sure to...", "you must manually free the resulting...", "watch out for race conditions with...", etc.

One example which comes to mind is some Javascript I wrote to estimated prices based on user-provided sliders and tick-boxes, and some formulas and constants which sales could edit in our CMS (basically, I had to implement a spreadsheet engine).

Recalculating after user input was pretty gnarly, since formulas could depend on each other in arbitrary ways, resulting in infinite loops and undefined variables when I tried to do it in a "straightforward" way. The "clever" solution I came up with was to evaluate formulas and values lazily: wrapping everything in thunks and using a memo table to turn exponential calculations into linear ones. It was small, simple and heavily-commented; but the team's unfamiliarity with concepts like lazy evaluation and memoising made it hard to get through code review.

Also, regarding "straightforward" or "verbose" code being "readable": it's certainly the case that any particular part of such code can be read and understood locally, but it can make the overall behaviour harder to understand. Just look at machine code: it's very verbose and straightforward: 'load address X into register A then add the value of register B', simple! Yet it's very hard to understand the "big picture" of what's going on. Making code more concise, either by simplifying it or at least abstracting away low-level, nitty-gritty details into well-named functions, can help with this.

When used well, "clever" code can reframe problems into a form which have very concise solutions; not because they've been code-golfed, but because there's so little left to say. This can mean the difference between a comprehensible system and a sprawling tangle of interfering patches. This may harm local reasoning in the short term, since it requires the reader to view things from that new perspective, when they may be expecting something else.

When used poorly, it results in things like nested ternaries, chasing conciseness without offering any deeper understanding of anything.

Re: How I review code

#82
post #60
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…

I've been doing a lot of Haskell on Codewars.com recently. This is exactly what I see. I write up a long solution, that uses the basics like pattern matching, heads of lists and such. The solution that has the most "Best practise" up-votes are usually something involving importing control.monad and other similar stuff.

Re: How I review code

#83
post #12

Earlier quoted context omitted.

Proving correctness is not the point of a code review. In fact it would be difficult to make such a proof in sufficiently complex software. Functional correctness is typically "proven" by tests. A code review ensures that the non-functional quality of the code is high. I.e. that the code is understandable/maintainable by someone other than the programmer himself, that there are no anti-patterns or dangerous-but-corre…

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.

Re: How I review code

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

That might be true in theory, but in practice most people I know with that title (& most with titles above it) do this sort of thing more often than junior developers.

Re: How I review code

#85

Earlier quoted context omitted.

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.

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.

Re: How I review code

#86
post #79

Earlier quoted context omitted.

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

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.

Re: How I review code

#87
post #70

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

> 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 JS, and I don't want to learn anything else right now", which is a terrible attitude for someone who wants to have a career in tech. Knowing more than one tool in the toolbox is a key skill, since there is no one-size-fits-all.

Jury is still out on Scala. It's a big language, but unclear if there's a production sweet spot or not compared to other JVM hosted languages.

The rest of the ones you listed have their definite sweet spots. There are tons of languages though, and most don't have one.

Re: How I review code

#88

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…

> as a back-burner project without official approval.

Can't this be seen as evaluation of a language?

Re: How I review code

#89

Earlier quoted context omitted.

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.

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.

Re: How I review code

#90

A conundrum for me is how to get other people to code review the way I want to be code reviewed? Particularly, I noticed code reviewers on my team are pretty pedantic, obsessed with correctness, and need to be explained why each change is okay. These are people that regularly write good quality code themselves, but there is a high amount of distrust. Why doesn't a team of talented programmers trust each other? (in ca…

It's probably not that they think you are wrong or can't be trusted. Think of it like defending a thesis. A typical code review process for me is to just ask the author a bunch of questions. If they have satisfying answers then great, the code goes in and we now have recorded answers for a bunch of questions about it (helpful if issues arise later on). But it shouldn't be surprising if there is something they haven't considered, despite being someone who writes quality code. Perhaps your team is guessing about correctness rather than asking about it and it comes across badly?
Post reply on HN