14 ms·

How I review code

234 points by peterstensmyr 9y ago

yeukhon 9y ago
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 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])) ^ This code is only for me to read so I didn't really care much about grammar... but a year from now I shouldn't have trouble understand the code in a minute or two. Some of my function/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's a bad code. Also, use more newlines... > I check every Github email I get; I make sure that I don’t get notified for everything that happens in the repo Not sure about others, but I am tired of reading PR notifications in my mailbox. I don't know how kernel developers can live with this. I have been thinking about just build a bot. * receives PUSH from GitHub * adds events to a queue * notifies based on priority * pings me once in a while to remind me that I have outstanding PRs to review (as reviewer and as author of the patch). If someone needs me to review right away, he/she can reach out to me directly in chat.
- yorwba 9y ago
  On a tangent: Assuming the first line is "def largest_square_plot(width, height):", you may be interested to know that your code computes the greatest common divisor [1]. If I'd been the one to write this function, I would have done it like: def largest_square_plot(width, height): """Computes the largest square to tile a plot of the given width and height.""" # because we want the grid of squares to fit exactly # the size of the squares needs to divide both width and height # to get the largest square, we use the greatest common divisor from math import gcd square_size = gcd(width, height) return (square_size, square_size) ... but now I realize that probably not everyone is intuitively familiar with the kind of math that involves the GCD, so your code is actually much more intuitive without that background. [1] https://en.wikipedia.org/wiki/Greatest_common_divisor https://en.wikipedia.org/wiki/Greatest_common_divisor
  - deleted 9y ago
    [deleted]
- xerophyte12932 9y ago
  A good counter argument to Comments: https://blog.codinghorror.com/coding-without-comments/ https://blog.codinghorror.com/coding-without-comments/
  - Stratoscope 9y ago
    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 = " + SquareRootApproximation(r) ); I'm all for this refactoring, but something was lost in the process. What kind of square root approximation is being used? Does the algorithm have a name? What would I search for if I wanted to read more about it? That information was in the original comment.
    - macobo 9y ago
      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 approximation function This is needed for rendering new polygons in renderer Foo in an efficient way as those don't need high degree of accuracy. The algorithm used was Newton-Raphson approximation, accuracy was chosen by initial testing: [[Test code here showing why a thing was chosen]] Potential pitfalls here include foo and bar. X and Y were also considered, but left out due to unclear benefit over the simpler algorithm. ``` With an editor with good `git blame` support (or using github to dig through the layers) this gives me a lot of confidence about reading code as I can go back in time and read what the author was thinking about originally. This way I can evaluate properly if conditions have changed, rather than worry about the next Chthulu comment that does not apply.
- yaccz 9y ago
  * receive email from GitHub / git-send-email / whatever * Maildir/new is queue * more emails in the same thread bump priority * emails marked unread are outstanding PRs
- snarf21 9y ago
  I recently read an article that was linked from HN comments on a different topic. It spoke about how software is not about code or documentation but rather about building complex mental models of the systems, which the author called the Theory Building View. It was interesting to read but one thing popped into my head while reading it. Too often we make comments about what a function does, which may be necessary but is not sufficient. My light bulb clicked on and I thought that what we really need is to document the function's reason for existing. This doesn't change even if the code inside does and is the thing that you actually really care about as it gives you some idea about the architecture.
- DenisM 9y ago
  FYI: If your variables are floats you will be splitting hair by the time this code terminates.
- darkerside 9y ago
  > 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 hint to remember the author was a senior engineer (who may "know more" than you do), but is it really something to keep in mind the entire time you review?
  - cobbzilla 9y ago
    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.
    - darkerside 9y ago
      The anti-pattern to avoid here is assuming code written by senior engineers is inherently "better" in some way than that by a junior. Yes, it typically is, but the code should speak for itself. Ad hominem assumptions add little value. Similar to blind testing in musical auditions. http://gap.hks.harvard.edu/orchestrating-impartiality-impact-“blind”-auditions-female-musicians http://gap.hks.harvard.edu/orchestrating-impartiality-impact...
anameaname 9y 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't a team of talented programmers trust each other? (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't amateurs, but it often seems like we're babies.)
- gizmo686 9y ago
  >obsessed with correctness, and need to be explained why each change is okay. Isn't this the point of code review? When I click accept on a code review, I am saying that I have looked over the change and believe that it is correct and okay. If I just arrive at the conclusion by saying "Joe wrote this, and I trust Joe" the there is no point in me reviewing it.
  - watwut 9y ago
    It continues with "and need to be explained why each change is okay". If reviewer suspects there is a bug, reviewer should check for it instead of having reviewee explain him or defend every detail. It is micromanagement this way - not just being similar or kinda like micromanagement, but it is literally it. If every five lines big code change in run of the mill fronted requires two hours long negotiation, then something is wrong. When it is unreadable and hard to see whether it works it is different thing, but then the complain in review should be some specific variant of "hard to read". I review mostly for architectural compliance and "bad idioms" or code smells. There is difference between not like I would write it and badly written mess and many programmers confuse them.
    - gizmo686 9y ago
      >If reviewer suspects there is a bug, reviewer should check for it instead of having reviewee explain him or defend every detail. It is not a matter of if the reviewer "suspects there is a bug", but rather a matter of if the reviewer is convinced that there is not a bug. If the reviewer needs to have someone explain why every minor code change is correct, they are either lazy or incompetent (or the code is badly written). Further, it is generally a bad idea to go to the person who wrote the code to explain why it is correct, as you are then much more likely to make the same mistake the original author made. However, being "okay" often goes beyond correctness, and into business decisions. Often times, these business are not documented (or if they are documented, it is in some management document that is not linked to from the code), so in order to review it, the reviewer has to ask the author what the bussiness consideration that led to the change was.
taylodl 9y ago
"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.
- scalesolved 9y ago
  Ha it does make me think of this tweet https://twitter.com/KevlinHenney/status/381021802941906944 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.
  - pqh 9y ago
    Text of the tweet so people don't have to click: A common fallacy is to assume authors of incomprehensible code will somehow be able to express themselves lucidly and clearly in comments.
  - mpweiher 9y ago
    There is a conundrum here: architectural mismatch. Often, the "right abstraction" for a problem is not call/return based. So you get to choose between having the right abstraction, and code that is simple when viewed with that abstraction in mind, but "weird". Or alternatively choose the wrong but better-supported abstraction and have code that is needlessly complex but "straightforward".
  - weego 9y ago
    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.
    - scalesolved 9y ago
soneca 9y 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). Any tips on how to improve this? Would a checklist help? Have a clear process on what to review first?
- driusan 9y ago
  Unless your current reviews are really superficial probably not. You can only review at your level of understanding, so if they're asking for more depth to your code reviews you probably need to develop a deeper understanding of the architecture and design of your codebase. A checklist would do the opposite of that in most cases.
- wiredfool 9y ago
  A checklist can help. (he says, and then does a mental one because there isn't one nearby). What I look for is: 1) What's the problem being solved? Does this look like a reasonable approach? Is the code pythonic (Obv: for python)? 2) What edge cases are there? Does this handle the important ones? Does it punt properly on the less important ones? 3) Look for a short list of bug classes that have come up in the project before that have lead to emergency patches. E.g. Decrefing, Checking mallocs, any exec sorts of things. (This is a clear application for a checklist) 4) Are there tests/documentation/other required fixtures and stuff? 5) Does the code generally match the style of the project? 1000) Code formating and whitespace and line wrapping and all that bikeshedding stuff. Feel free to short circuit anywhere once it becomes clear that there's more work required.
  - kripke 9y ago
    1000 should really be handled by automated tools. Takes useless burden from the reviewer, and emotionally easier for both sides too.
    - wiredfool 9y ago
      It can be, especially at a company. (But then watch the bikeshed discussion on the tools. And PEP8 is a guideline, not a set of hard and fast rules. Beautiful is better than ugly and all that. ). What I see as one of 5 maintainers of an open source project is that when a review comes back with a bunch of formatting comments, it's because the reviewer didn't step back and see the other parts. Raymond Hettinger has a talk where he discusses it, but it's the sort of feedback that can be given in almost any case and can obscure the more fundamental issues with the code.
skate22 9y ago
My code is well commented. //eslint-disable-line all over the place.
- lsadam0 9y ago
  Invoking eslint-disable needs to have comment of it's own justifying why the line in question is being skipped. What use is the linter if we just disable it everytime it complains?
  - skate22 9y ago
    I dont have permission to remove lint rules but some of them are absurd. I refuse to remove 'extrenious' parenthesis that make the code more readable to junior devs who may not know the language specific order of evaluation in a logical expression.
dingo_bat 9y ago
> I’d rather read ten lines of verbose-but-understandable code than someone’s ninja-tastic one-liner that involves four nested ternaries. 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've tried this with many reviewers but nope.
nimbix 9y 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't actually go through the code.
- ryanianian 9y ago
  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.
pmcollins 9y ago
> 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 line.
- dboreham 9y ago
  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 <language-du-jour>. He says it is so much better than <dinosaur-languages>. 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 it and is insistent. Do you tell them no, or do you let them run? Well now you have N+1 languages. Iterate. Some engineer in your organization develops something that turns out to be useful, as a back-burner project without official approval. They do that in whatever language they think will look good on their resume. Do you pay to have that thing re-written in one of the house languages or do you let it ride and add it to the mix. N+1 languages. Iterate. And...in general good luck with not "allowing this" in the context of software developers. Cat herding and all that.
  - cobbzilla 9y ago
    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.
    - volkk 9y ago
      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
DarkVador 9y ago
I don't understand why you need to know the progamer behind the code ? You need to be totaly impartial when you judge something. So i think it's a wrong way to review the code. You don't need the WHO but the WHY.
chriswarbo 9y ago
> 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 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.
thetruthseeker1 9y 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.
agentgt 9y ago
> 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.
j05huaNathaniel 9y ago
My motto is always do the least amount of work necessary. This usually ends up in small code fragments. My biggest concern with PRs is that they get too big to be reviewed meaningfully. Sometimes developers just "rubber-stamp" an approval. One way I've forced others to review my code is to put early code up in a PR for feedback. It allows others to see the process. It also drives the point home that we are all human and don't magically poop out great code.
bwest87 9y ago
Something the author doesn'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's great. We don't do it for all PR's, but anything medium sized or above, or even small one'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't a natural outgrowth. We make it happen, but I think it's worth it.
- uremog 9y ago
  That sounds very similar to Rubber Duck Debugging.

234 points by peterstensmyr 9y ago

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 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])) ^ This code is only for me to read so I didn't really care much about grammar... but a year from now I shouldn't have trouble understand the code in a minute or two. Some of my function/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's a bad code. Also, use more newlines... > I check every Github email I get; I make sure that I don’t get notified for everything that happens in the repo Not sure about others, but I am tired of reading PR notifications in my mailbox. I don't know how kernel developers can live with this. I have been thinking about just build a bot. * receives PUSH from GitHub * adds events to a queue * notifies based on priority * pings me once in a while to remind me that I have outstanding PRs to review (as reviewer and as author of the patch). If someone needs me to review right away, he/she can reach out to me directly in chat.

yorwba 9y ago
On a tangent: Assuming the first line is "def largest_square_plot(width, height):", you may be interested to know that your code computes the greatest common divisor [1]. If I'd been the one to write this function, I would have done it like: def largest_square_plot(width, height): """Computes the largest square to tile a plot of the given width and height.""" # because we want the grid of squares to fit exactly # the size of the squares needs to divide both width and height # to get the largest square, we use the greatest common divisor from math import gcd square_size = gcd(width, height) return (square_size, square_size) ... but now I realize that probably not everyone is intuitively familiar with the kind of math that involves the GCD, so your code is actually much more intuitive without that background. [1] https://en.wikipedia.org/wiki/Greatest_common_divisor https://en.wikipedia.org/wiki/Greatest_common_divisor
- deleted 9y ago
  [deleted]
xerophyte12932 9y ago
A good counter argument to Comments: https://blog.codinghorror.com/coding-without-comments/ https://blog.codinghorror.com/coding-without-comments/
- Stratoscope 9y ago
  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 = " + SquareRootApproximation(r) ); I'm all for this refactoring, but something was lost in the process. What kind of square root approximation is being used? Does the algorithm have a name? What would I search for if I wanted to read more about it? That information was in the original comment.
  - macobo 9y ago
    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 approximation function This is needed for rendering new polygons in renderer Foo in an efficient way as those don't need high degree of accuracy. The algorithm used was Newton-Raphson approximation, accuracy was chosen by initial testing: [[Test code here showing why a thing was chosen]] Potential pitfalls here include foo and bar. X and Y were also considered, but left out due to unclear benefit over the simpler algorithm. ``` With an editor with good `git blame` support (or using github to dig through the layers) this gives me a lot of confidence about reading code as I can go back in time and read what the author was thinking about originally. This way I can evaluate properly if conditions have changed, rather than worry about the next Chthulu comment that does not apply.
yaccz 9y ago
* receive email from GitHub / git-send-email / whatever * Maildir/new is queue * more emails in the same thread bump priority * emails marked unread are outstanding PRs
snarf21 9y ago
I recently read an article that was linked from HN comments on a different topic. It spoke about how software is not about code or documentation but rather about building complex mental models of the systems, which the author called the Theory Building View. It was interesting to read but one thing popped into my head while reading it. Too often we make comments about what a function does, which may be necessary but is not sufficient. My light bulb clicked on and I thought that what we really need is to document the function's reason for existing. This doesn't change even if the code inside does and is the thing that you actually really care about as it gives you some idea about the architecture.
DenisM 9y ago
FYI: If your variables are floats you will be splitting hair by the time this code terminates.
darkerside 9y ago
> 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 hint to remember the author was a senior engineer (who may "know more" than you do), but is it really something to keep in mind the entire time you review?
- cobbzilla 9y ago
  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.
  - darkerside 9y ago
    The anti-pattern to avoid here is assuming code written by senior engineers is inherently "better" in some way than that by a junior. Yes, it typically is, but the code should speak for itself. Ad hominem assumptions add little value. Similar to blind testing in musical auditions. http://gap.hks.harvard.edu/orchestrating-impartiality-impact-“blind”-auditions-female-musicians http://gap.hks.harvard.edu/orchestrating-impartiality-impact...

anameaname 9y 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't a team of talented programmers trust each other? (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't amateurs, but it often seems like we're babies.)

gizmo686 9y ago
>obsessed with correctness, and need to be explained why each change is okay. Isn't this the point of code review? When I click accept on a code review, I am saying that I have looked over the change and believe that it is correct and okay. If I just arrive at the conclusion by saying "Joe wrote this, and I trust Joe" the there is no point in me reviewing it.
- watwut 9y ago
  It continues with "and need to be explained why each change is okay". If reviewer suspects there is a bug, reviewer should check for it instead of having reviewee explain him or defend every detail. It is micromanagement this way - not just being similar or kinda like micromanagement, but it is literally it. If every five lines big code change in run of the mill fronted requires two hours long negotiation, then something is wrong. When it is unreadable and hard to see whether it works it is different thing, but then the complain in review should be some specific variant of "hard to read". I review mostly for architectural compliance and "bad idioms" or code smells. There is difference between not like I would write it and badly written mess and many programmers confuse them.
  - gizmo686 9y ago
    >If reviewer suspects there is a bug, reviewer should check for it instead of having reviewee explain him or defend every detail. It is not a matter of if the reviewer "suspects there is a bug", but rather a matter of if the reviewer is convinced that there is not a bug. If the reviewer needs to have someone explain why every minor code change is correct, they are either lazy or incompetent (or the code is badly written). Further, it is generally a bad idea to go to the person who wrote the code to explain why it is correct, as you are then much more likely to make the same mistake the original author made. However, being "okay" often goes beyond correctness, and into business decisions. Often times, these business are not documented (or if they are documented, it is in some management document that is not linked to from the code), so in order to review it, the reviewer has to ask the author what the bussiness consideration that led to the change was.

taylodl 9y ago

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

scalesolved 9y ago
Ha it does make me think of this tweet https://twitter.com/KevlinHenney/status/381021802941906944 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.
- pqh 9y ago
  Text of the tweet so people don't have to click: A common fallacy is to assume authors of incomprehensible code will somehow be able to express themselves lucidly and clearly in comments.
- mpweiher 9y ago
  There is a conundrum here: architectural mismatch. Often, the "right abstraction" for a problem is not call/return based. So you get to choose between having the right abstraction, and code that is simple when viewed with that abstraction in mind, but "weird". Or alternatively choose the wrong but better-supported abstraction and have code that is needlessly complex but "straightforward".
- weego 9y ago
  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.
  - scalesolved 9y ago

soneca 9y 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). Any tips on how to improve this? Would a checklist help? Have a clear process on what to review first?

driusan 9y ago
Unless your current reviews are really superficial probably not. You can only review at your level of understanding, so if they're asking for more depth to your code reviews you probably need to develop a deeper understanding of the architecture and design of your codebase. A checklist would do the opposite of that in most cases.
wiredfool 9y ago
A checklist can help. (he says, and then does a mental one because there isn't one nearby). What I look for is: 1) What's the problem being solved? Does this look like a reasonable approach? Is the code pythonic (Obv: for python)? 2) What edge cases are there? Does this handle the important ones? Does it punt properly on the less important ones? 3) Look for a short list of bug classes that have come up in the project before that have lead to emergency patches. E.g. Decrefing, Checking mallocs, any exec sorts of things. (This is a clear application for a checklist) 4) Are there tests/documentation/other required fixtures and stuff? 5) Does the code generally match the style of the project? 1000) Code formating and whitespace and line wrapping and all that bikeshedding stuff. Feel free to short circuit anywhere once it becomes clear that there's more work required.
- kripke 9y ago
  1000 should really be handled by automated tools. Takes useless burden from the reviewer, and emotionally easier for both sides too.
  - wiredfool 9y ago
    It can be, especially at a company. (But then watch the bikeshed discussion on the tools. And PEP8 is a guideline, not a set of hard and fast rules. Beautiful is better than ugly and all that. ). What I see as one of 5 maintainers of an open source project is that when a review comes back with a bunch of formatting comments, it's because the reviewer didn't step back and see the other parts. Raymond Hettinger has a talk where he discusses it, but it's the sort of feedback that can be given in almost any case and can obscure the more fundamental issues with the code.

skate22 9y ago

My code is well commented. //eslint-disable-line all over the place.

lsadam0 9y ago
Invoking eslint-disable needs to have comment of it's own justifying why the line in question is being skipped. What use is the linter if we just disable it everytime it complains?
- skate22 9y ago
  I dont have permission to remove lint rules but some of them are absurd. I refuse to remove 'extrenious' parenthesis that make the code more readable to junior devs who may not know the language specific order of evaluation in a logical expression.

dingo_bat 9y ago

> I’d rather read ten lines of verbose-but-understandable code than someone’s ninja-tastic one-liner that involves four nested ternaries. 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've tried this with many reviewers but nope.

nimbix 9y 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't actually go through the code.

ryanianian 9y ago
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.

pmcollins 9y ago

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

dboreham 9y ago
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 <language-du-jour>. He says it is so much better than <dinosaur-languages>. 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 it and is insistent. Do you tell them no, or do you let them run? Well now you have N+1 languages. Iterate. Some engineer in your organization develops something that turns out to be useful, as a back-burner project without official approval. They do that in whatever language they think will look good on their resume. Do you pay to have that thing re-written in one of the house languages or do you let it ride and add it to the mix. N+1 languages. Iterate. And...in general good luck with not "allowing this" in the context of software developers. Cat herding and all that.
- cobbzilla 9y ago
  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.
  - volkk 9y ago
    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

DarkVador 9y ago

I don't understand why you need to know the progamer behind the code ? You need to be totaly impartial when you judge something. So i think it's a wrong way to review the code. You don't need the WHO but the WHY.

chriswarbo 9y ago

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

thetruthseeker1 9y 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.

agentgt 9y ago

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

j05huaNathaniel 9y ago

My motto is always do the least amount of work necessary. This usually ends up in small code fragments. My biggest concern with PRs is that they get too big to be reviewed meaningfully. Sometimes developers just "rubber-stamp" an approval. One way I've forced others to review my code is to put early code up in a PR for feedback. It allows others to see the process. It also drives the point home that we are all human and don't magically poop out great code.

bwest87 9y ago

Something the author doesn'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's great. We don't do it for all PR's, but anything medium sized or above, or even small one'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't a natural outgrowth. We make it happen, but I think it's worth it.

uremog 9y ago
That sounds very similar to Rubber Duck Debugging.