TE
TechEcho
Home24h TopNewestBestAskShowJobs
GitHubTwitter
Home

TechEcho

A tech news platform built with Next.js, providing global tech news and discussions.

GitHubTwitter

Home

HomeNewestBestAskShowJobs

Resources

HackerNews APIOriginal HackerNewsNext.js

© 2025 TechEcho. All rights reserved.

How I review code

234 pointsby peterstensmyrover 7 years ago

13 comments

taylodlover 7 years ago
<i>&quot;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.&quot;</i><p>Ha! That&#x27;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&#x27;t there yet.
评论 #16222789 未加载
评论 #16222089 未加载
评论 #16222637 未加载
评论 #16224064 未加载
评论 #16223251 未加载
评论 #16223644 未加载
yeukhonover 7 years ago
I echo the author&#x27;s point in &quot;Review the code with its author in mind&quot;.<p>Without comments, sometimes it&#x27;s really really difficult to navigate the code. I have been adding more comments than ever: don&#x27;t assume every line is obvious, write a comment to explain what the next few lines really do.<p><pre><code> # base case: stop dividing when we find the largest square. if width == height: return width, height else: # otherwise, we know that we can break the land into several # pieces, some are already square-sized, but there must be # left over, which is the difference of the width and height, # and can be divided again. remain = abs(width - height) return largest_square_plot(remain, min([width, height])) </code></pre> ^ This code is only for me to read so I didn&#x27;t really care much about grammar... but a year from now I shouldn&#x27;t have trouble understand the code in a minute or two.<p>Some of my function&#x2F;method has a pretty long docstring which may include explaining the rationale, and perhaps even some ascii diagrams. If you have trouble understand a piece of code after a few passes, that&#x27;s a bad code. Also, use more newlines...<p>&gt; I check every Github email I get; I make sure that I don’t get notified for everything that happens in the repo<p>Not sure about others, but I am tired of reading PR notifications in my mailbox. I don&#x27;t know how kernel developers can live with this.<p>I have been thinking about just build a bot.<p>* receives PUSH from GitHub<p>* adds events to a queue<p>* notifies based on priority<p>* pings me once in a while to remind me that I have outstanding PRs to review (as reviewer and as author of the patch).<p>If someone needs me to review right away, he&#x2F;she can reach out to me directly in chat.
评论 #16221984 未加载
评论 #16222725 未加载
评论 #16221485 未加载
评论 #16222896 未加载
评论 #16222757 未加载
评论 #16222682 未加载
anameanameover 7 years ago
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&#x27;t a team of talented programmers trust each other?<p>(in case it needs to be stated, to date, I have reviewed about as much code as I have written, upwards of 100k lines, as have most of the other people on my team. We aren&#x27;t amateurs, but it often seems like we&#x27;re babies.)
评论 #16221128 未加载
评论 #16221387 未加载
评论 #16221171 未加载
评论 #16221213 未加载
评论 #16221919 未加载
评论 #16221741 未加载
评论 #16221131 未加载
评论 #16221169 未加载
评论 #16222699 未加载
评论 #16221900 未加载
评论 #16222304 未加载
评论 #16224635 未加载
评论 #16223768 未加载
评论 #16223844 未加载
pmcollinsover 7 years ago
&gt; We have repositories for the PHP backend, our database schemas, our iOS (Swift&#x2F;Obj-C) and Android (Java&#x2F;Kotlin) mobile apps, infrastructure projects written in Go, C&#x2F;C++, Lua, Ruby, Perl, and many other projects written in Scala, Node.js, Python, and more<p>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.
评论 #16222679 未加载
评论 #16223155 未加载
评论 #16222927 未加载
评论 #16223075 未加载
评论 #16227144 未加载
评论 #16222685 未加载
skate22over 7 years ago
My code is well commented. &#x2F;&#x2F;eslint-disable-line all over the place.
评论 #16222790 未加载
nimbixover 7 years ago
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&#x27;t actually go through the code.
评论 #16223165 未加载
bwest87over 7 years ago
Something the author doesn&#x27;t bring up, but that we started doing about 9 months ago at my company, is synchronous reviews. Meaning the committer is on the phone or in person with the reviewer. It&#x27;s great. We don&#x27;t do it for all PR&#x27;s, but anything medium sized or above, or even small one&#x27;s if they involve critical logic. The way we usually do it is the committer walks through the changes with the reviewer. Often the committer will realize their own ways of improving the code. And with the added context, the reviewer can often provide better feedback. Plus the X factor of just two people talking who come up with ideas, improvements, etc. And half our team is remote, so this wasn&#x27;t a natural outgrowth. We make it happen, but I think it&#x27;s worth it.
评论 #16226929 未加载
sonecaover 7 years ago
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).<p>Any tips on how to improve this?<p>Would a checklist help? Have a clear process on what to review first?
评论 #16222623 未加载
评论 #16226680 未加载
评论 #16222503 未加载
chriswarboover 7 years ago
&gt; 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.<p>&quot;Clear&quot; and &quot;clever&quot; aren&#x27;t in opposition, and likewise &quot;verbose&quot; and &quot;understandable&quot; aren&#x27;t correlated.<p>I think this characterisation, and especially the example, shows a lowest-common-denominator straw man of &quot;clever one-liners&quot; 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&#x27;t say what those &quot;ten lines of verbose-but-understandable code&quot; would be, but given the context I took it to mean &quot;exactly the same solution, but written with intermediate variables or if&#x2F;else blocks instead&quot;.<p>This seems like an analogous situation to <a href="https:&#x2F;&#x2F;wiki.haskell.org&#x2F;Wadler&#x27;s_Law" rel="nofollow">https:&#x2F;&#x2F;wiki.haskell.org&#x2F;Wadler&#x27;s_Law</a> 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&#x2F;new lines, etc.).<p>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 &amp;&amp; !isAdmin` seems pretty clear to me; playing around with truth tables, I found that `!(loggedIn -&gt; isAdmin)` is apparently equivalent, but it seems rather cryptic to me. This is more obvious if intermediate variables are used, since they&#x27;re easier to name if they&#x27;re meaningful.<p>In any case, compressing code by encoding the same thing with different symbols doesn&#x27;t make something &quot;clever&quot;. It&#x27;s a purely mechanical process which doesn&#x27;t involve any insights into the domain.<p>To me, code is &quot;clever&quot; if it works by exploiting some non-obvious structure&#x2F;pattern in the domain or system. For example, code which calculates a particular index&#x2F;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&#x27;s device, exceptions for control flow, etc.).<p>Such &quot;clever&quot; code is <i>often</i> more terse than a &quot;straightforward&quot; alternative, but that&#x27;s a side-effect of the &quot;cleverness&quot; (finding an existing thing which behaves like the thing we want) rather than the goal.<p>If the alternative to some &quot;clever&quot; code is &quot;10 lines of verbose but understandable code&quot; then it&#x27;s probably not that clever; so it&#x27;s probably a safe bet to go with the latter. The <i>real</i> issues with clever code are:<p>- 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?<p>- How hard it is to understand. Even if it&#x27;s non-obvious, can it be understood after a moment&#x27;s pondering; or does it require working through a textbook and several research papers?<p>- 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&#x27;s harder to understand. Enlightening insights can change the way we understand the system&#x2F;domain, which may have many benefits going forward. Incidental insights are one-off tricks that won&#x27;t help us in the future.<p>- How difficult it would be to replace; or whether it&#x27;s possible to replace at all.<p>This last point is what annoys me in naive &quot;clever vs verbose&quot; debates, and prompted this rant, since it&#x27;s often assumed that the only difference is line count. To me, the best &quot;clever&quot; code isn&#x27;t that which reduces <i>its own</i> line count; it&#x27;s the code which <i>removes problems entirely</i>; i.e. where the alternative has caveats like &quot;before calling, make sure to...&quot;, &quot;you must manually free the resulting...&quot;, &quot;watch out for race conditions with...&quot;, etc.<p>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).<p>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 &quot;straightforward&quot; way. The &quot;clever&quot; 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&#x27;s unfamiliarity with concepts like lazy evaluation and memoising made it hard to get through code review.<p>Also, regarding &quot;straightforward&quot; or &quot;verbose&quot; code being &quot;readable&quot;: it&#x27;s certainly the case that any <i>particular</i> part of such code can be read and understood <i>locally</i>, but it can make the <i>overall</i> behaviour harder to understand. Just look at machine code: it&#x27;s very verbose and straightforward: &#x27;load address X into register A then add the value of register B&#x27;, simple! Yet it&#x27;s very hard to understand the &quot;big picture&quot; of what&#x27;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.<p>When used well, &quot;clever&quot; code can reframe problems into a form which have very concise solutions; not because they&#x27;ve been code-golfed, but because there&#x27;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.<p>When used poorly, it results in things like nested ternaries, chasing conciseness without offering any deeper understanding of anything.
评论 #16224743 未加载
thetruthseeker1over 7 years ago
I don’t know how much time he spends code reviewing. But if at the end of the day he wants anybody to get the complete context of what the change entails by looking at the PR... I would think the code review process is more elaborate and time consuming than many companies can afford.
agentgtover 7 years ago
&gt; <i>clever code is often difficult to read and understand later</i><p>I have seen this many times and they are actually usually talented developers that are just not used to working in groups....<p>but what I have seen more often (back when I did code reviews)... is lazy copy and pasting or something analogous.
DarkVadorover 7 years ago
I don&#x27;t understand why you need to know the progamer behind the code ? You need to be totaly impartial when you judge something.<p>So i think it&#x27;s a wrong way to review the code. You don&#x27;t need the WHO but the WHY.
dingo_batover 7 years ago
&gt; I’d rather read ten lines of verbose-but-understandable code than someone’s ninja-tastic one-liner that involves four nested ternaries.<p>One of my pet peeves is the inability to solve a K-Map for 6 variables and do stuff based on the resulting boolean expression. Just because it is utterly unreadable. I&#x27;ve tried this with many reviewers but nope.