5 ms·

First thing I review is readability. Code should be readable. Once code is readable it makes reviewing the rest much easier. Readable code is more maintaina

First thing I review is readability. Code should be readable. Once code is readable it makes reviewing the rest much easier. Readable code is more maintainable and problems jump out at you. Stuff other than readability is important, but if you focus on readability it makes the rest of the review go very smoothly.

nine_zeros 5y ago
If the code is doing the wrong thing in the first place, no amount of nitpicking on readability will save time. First pass: Functional correctness Second pass: Better ways to do the same thing (optional) Third pass: your favorite nitpicks.
- pc86 5y ago
  This structure (especially the optionality of the second pass) is how you end up with perfectly readable code iterating over List<T> when a hash map would suffice and be orders of magnitude more performant. Nobody can do it perfectly, but I think trying to keep personal nitpicks out of reviews as much as possible - ideally by codifying team nitpicks in automated formatting/.prettierrc/whatever - lets people focus on things that matter: what the code does and how it does it.
  - watwut 5y ago
    Then again, that one is super easy to find and fix if it turns out to actually be problem.
  - TheCoelacanth 5y ago
    In perfectly readable code, it's easy to notice performance problems. In my experience, unreadable code is usually where the worst performance problems are, because that's where they are the hardest to find and fix. You seem to be conflating readability with formatting, which is not what I mean by readability. Poor and inconsistent formatting adds some friction to reading, but it's a relatively minor part of readability.
- sgeisenh 5y ago
  I think you're missing the point: it's incredibly hard to determine functional correctness of unreadable code. So the first priority is getting the change to a readable state. I'm not talking about nits, I'm talking about minimizing the cognitive overhead to truly understand what the code is doing. That being said, it is also easy to identify nits during this phase. In codebases that have collections of best practices, this often helps to ensure that the change is expressed in terms of a common vocabulary. Once this pass is done, the reviewer can usually better understand the intent of the changes.
  - nine_zeros 5y ago
    > I think you're missing the point: it's incredibly hard to determine functional correctness of unreadable code. There is a spectrum between style differences and absolutely unreadable code. If it is absolutely unreadable, sure, send it back to be readable. But if it is merely styling issues, I'd encourage you to understand that you are not saving any time by focusing on styling before functionality.
    - TheCoelacanth 5y ago
      Readability has very little to do with styling. It's about clearly and concisely expressing ideas. Leave styling to linters. Reviewers should be focusing on how the code is communicating.
    - chimprich 5y ago
      > I'd encourage you to understand that you are not saving any time by focusing on styling before functionality. Far more developer time is spent reading code than writing it. If you can speed up the time required to understand a piece a code by improving the style then it's almost always worth it. For a professional software engineer, just above "absolutely unreadable" is far too low a bar to aim for.
      - nine_zeros 5y ago
        I am still not convinced that readability comes before functionality. Imagine a developer building a PR for 2 weeks, then reviews back and forth for 2 more weeks. Now 4 weeks have passed and only now the reviewer reviews the functionality - only to find that the entire implementation is wrong/could be done in a better way. What a waste of 4 weeks of both the author and the reviewer! This could have been short-circuited very early in the process. On my team, we default to early feedback on functionality and let the CI enforce what it can. Everything else is debatable.
- mdtusz 5y ago
  As a counter argument, it's much easier for a developer to write code that does the right thing when that code is readable. Too often I see code that is _maybe_ correct, but the reviewer can't actually be sure of it without manually testing it, and the chances that a future reader in 4 months will have any idea what it does or why are extremely low. Good variable and function names get you 90% of the way there yet for some reason cryptic code is still all too common in the world. To me, reviewing for readability isn't nitpicking - it's equivalent to a mechanical engineering design review pointing out that design for maintenance or design for assembly has been entirely ignored.
- thanhhaimai 5y ago
  It's assumed in your post that readability review is nitpicking. It's actually about making the code idiomatic, consistent, predictable, quickly understandable, and low mental stress while reading. The "nitpicking" parts you referred mostly is about ensuring code consistency and predictability. But there are more to readability than that: 1) did the code use a third party library while a standard lib is sufficient? Is there a way to use native language features (e.g. list comprehension) to make the code more idiomatic to people familiar with the language? 2) are the namings make sense in the business context? Can a new hire read just the public interface and be able to guess the usage? 3) do people have to jump around, full-text-search the code base to understand what's going on? Is automatic dependencies injection being overused? How many things people need to keep in their head to be able to follow this code? It's all readability there, and I'd argue it's not only about style or formatting.
- GaveUp 5y ago
  I don't know if I agree that the second pass is optional. I've found that pass is the one I've seen have the most benefit, particularly with newer developers. It serves two purposes when done well. First it gives you an idea of how well thought out the implementation was (i.e. was this a quick hack to just finish a asked for requirement) or was a best design targeted. It also helps newer developers develop a voice. Often, at the start, newer devs will take what a more senior dev says as gospel, but by striking up a conversation and, in some ways, making them defend their choices it can help build confidence and that voice to speak up when something doesn't seem right. Second, I've found it a good way to introduce people to new approaches to accomplishing tasks. Not everyone spends their off hours studying patterns and practices and rarely is there time during a work day to do this properly so code reviews are a natural place to bring these things up as there's concrete comparisons and examples to work with. That helps spark a dev's interest to look in to the topics further.
- mpalczewski 5y ago
  yes, the strawman of nitpicking has been killed.
thanhhaimai 5y ago
+1, I also focus first on readability while reviewing code. Software engineering is a collaborative process. I don't write code for the machine; I write it for my colleagues and my future self to read. Code we write 6 months ago looks like someone else code. Readability is important unless you're convinced that it's prototype/throwaway code. After readability then I'd look for tests. It's a theme: good tests are also meant to be read as documentation. Beside ensuring correctness, it's also the example my colleagues or future self can refer for the module usage.
convolvatron 5y ago
unfortunately, 'readability' is fairly subjective. I was part of a group recently that spent a good couple hours every week arguing whether 'ctxt' or 'context' was more readable. when confronted, they explained to me as one would a child, that they cared deeply about code quality
- _nhynes 5y ago
  That's ridiculous. Obviously `ctx` is the most readable.
  - maze-le 5y ago
    Its obvious from the context that "c" is the only variable name that should represent a context.
- deleted 5y ago
  [deleted]
- deleted 5y ago
  [deleted]
- mpalczewski 5y ago
  > when confronted, they explained to me as one would a child, that they cared deeply about code quality these people sound useless. Readability as it applies to code is how easy it is to read the code and understand it in relation to how complicated the problem domain is.
- Bjartr 5y ago
  The best choice is whichever is more consistent with the surrounding codebase. For an outsider to the codebase, you could argue for 'context' being better, But to someone familiar to the codebase, if contexts are always in a variable 'ctxt', then 'ctxt' will instantly parse correctly without a hitch since it'll be interpreted as a single symbol.
- TheCoelacanth 5y ago
  There's a simple test for readability. Someone familiar with the codebase reads the code and tries to understand what it does (a.k.a. a code review). If it takes a low effort, then it's high readability. If it takes a high effort, then it's low readability.
seadan83 5y ago
This is where I have found review to be really tricky. I have reviewed so much unreadable code that I lose faith sometimes. More to the point, readability issues are often perceived as nitpicks. Eventually after a few rounds of readability issues being addressed I can tell what the actual code is doing. At that point the major suggestions and some rework comes out, but now the author feels burned, nitpicked, and now instead of being done "their perfectly functional" code needs more work and the whole thing is not done. Do this a few times and relationships begin to break down. Hence, I now try really hard to ignore readability until the end. I don't know if this partly just hostile developers, different understanding of code. Ie, is code for humans to read, or for machines to execute? Or if it's dealing with feature factory developers that just want to keep adding despite how brittle the code is becoming. I presume the problem is somewhere in between, but to another extent the normalcy of the absurd where it is normal to spend 5 minutes per line of code to understand it, it is normal to havk in features and do manual regression testing, etc..

nine_zeros 5y ago

If the code is doing the wrong thing in the first place, no amount of nitpicking on readability will save time. First pass: Functional correctness Second pass: Better ways to do the same thing (optional) Third pass: your favorite nitpicks.

pc86 5y ago
This structure (especially the optionality of the second pass) is how you end up with perfectly readable code iterating over List<T> when a hash map would suffice and be orders of magnitude more performant. Nobody can do it perfectly, but I think trying to keep personal nitpicks out of reviews as much as possible - ideally by codifying team nitpicks in automated formatting/.prettierrc/whatever - lets people focus on things that matter: what the code does and how it does it.
- watwut 5y ago
  Then again, that one is super easy to find and fix if it turns out to actually be problem.
- TheCoelacanth 5y ago
  In perfectly readable code, it's easy to notice performance problems. In my experience, unreadable code is usually where the worst performance problems are, because that's where they are the hardest to find and fix. You seem to be conflating readability with formatting, which is not what I mean by readability. Poor and inconsistent formatting adds some friction to reading, but it's a relatively minor part of readability.
sgeisenh 5y ago
I think you're missing the point: it's incredibly hard to determine functional correctness of unreadable code. So the first priority is getting the change to a readable state. I'm not talking about nits, I'm talking about minimizing the cognitive overhead to truly understand what the code is doing. That being said, it is also easy to identify nits during this phase. In codebases that have collections of best practices, this often helps to ensure that the change is expressed in terms of a common vocabulary. Once this pass is done, the reviewer can usually better understand the intent of the changes.
- nine_zeros 5y ago
  > I think you're missing the point: it's incredibly hard to determine functional correctness of unreadable code. There is a spectrum between style differences and absolutely unreadable code. If it is absolutely unreadable, sure, send it back to be readable. But if it is merely styling issues, I'd encourage you to understand that you are not saving any time by focusing on styling before functionality.
  - TheCoelacanth 5y ago
    Readability has very little to do with styling. It's about clearly and concisely expressing ideas. Leave styling to linters. Reviewers should be focusing on how the code is communicating.
  - chimprich 5y ago
    > I'd encourage you to understand that you are not saving any time by focusing on styling before functionality. Far more developer time is spent reading code than writing it. If you can speed up the time required to understand a piece a code by improving the style then it's almost always worth it. For a professional software engineer, just above "absolutely unreadable" is far too low a bar to aim for.
    - nine_zeros 5y ago
      I am still not convinced that readability comes before functionality. Imagine a developer building a PR for 2 weeks, then reviews back and forth for 2 more weeks. Now 4 weeks have passed and only now the reviewer reviews the functionality - only to find that the entire implementation is wrong/could be done in a better way. What a waste of 4 weeks of both the author and the reviewer! This could have been short-circuited very early in the process. On my team, we default to early feedback on functionality and let the CI enforce what it can. Everything else is debatable.
mdtusz 5y ago
As a counter argument, it's much easier for a developer to write code that does the right thing when that code is readable. Too often I see code that is _maybe_ correct, but the reviewer can't actually be sure of it without manually testing it, and the chances that a future reader in 4 months will have any idea what it does or why are extremely low. Good variable and function names get you 90% of the way there yet for some reason cryptic code is still all too common in the world. To me, reviewing for readability isn't nitpicking - it's equivalent to a mechanical engineering design review pointing out that design for maintenance or design for assembly has been entirely ignored.
thanhhaimai 5y ago
It's assumed in your post that readability review is nitpicking. It's actually about making the code idiomatic, consistent, predictable, quickly understandable, and low mental stress while reading. The "nitpicking" parts you referred mostly is about ensuring code consistency and predictability. But there are more to readability than that: 1) did the code use a third party library while a standard lib is sufficient? Is there a way to use native language features (e.g. list comprehension) to make the code more idiomatic to people familiar with the language? 2) are the namings make sense in the business context? Can a new hire read just the public interface and be able to guess the usage? 3) do people have to jump around, full-text-search the code base to understand what's going on? Is automatic dependencies injection being overused? How many things people need to keep in their head to be able to follow this code? It's all readability there, and I'd argue it's not only about style or formatting.
GaveUp 5y ago
I don't know if I agree that the second pass is optional. I've found that pass is the one I've seen have the most benefit, particularly with newer developers. It serves two purposes when done well. First it gives you an idea of how well thought out the implementation was (i.e. was this a quick hack to just finish a asked for requirement) or was a best design targeted. It also helps newer developers develop a voice. Often, at the start, newer devs will take what a more senior dev says as gospel, but by striking up a conversation and, in some ways, making them defend their choices it can help build confidence and that voice to speak up when something doesn't seem right. Second, I've found it a good way to introduce people to new approaches to accomplishing tasks. Not everyone spends their off hours studying patterns and practices and rarely is there time during a work day to do this properly so code reviews are a natural place to bring these things up as there's concrete comparisons and examples to work with. That helps spark a dev's interest to look in to the topics further.
mpalczewski 5y ago
yes, the strawman of nitpicking has been killed.

thanhhaimai 5y ago

+1, I also focus first on readability while reviewing code. Software engineering is a collaborative process. I don't write code for the machine; I write it for my colleagues and my future self to read. Code we write 6 months ago looks like someone else code. Readability is important unless you're convinced that it's prototype/throwaway code. After readability then I'd look for tests. It's a theme: good tests are also meant to be read as documentation. Beside ensuring correctness, it's also the example my colleagues or future self can refer for the module usage.

convolvatron 5y ago

unfortunately, 'readability' is fairly subjective. I was part of a group recently that spent a good couple hours every week arguing whether 'ctxt' or 'context' was more readable. when confronted, they explained to me as one would a child, that they cared deeply about code quality

_nhynes 5y ago
That's ridiculous. Obviously `ctx` is the most readable.
- maze-le 5y ago
  Its obvious from the context that "c" is the only variable name that should represent a context.
deleted 5y ago
[deleted]
deleted 5y ago
[deleted]
mpalczewski 5y ago
> when confronted, they explained to me as one would a child, that they cared deeply about code quality these people sound useless. Readability as it applies to code is how easy it is to read the code and understand it in relation to how complicated the problem domain is.
Bjartr 5y ago
The best choice is whichever is more consistent with the surrounding codebase. For an outsider to the codebase, you could argue for 'context' being better, But to someone familiar to the codebase, if contexts are always in a variable 'ctxt', then 'ctxt' will instantly parse correctly without a hitch since it'll be interpreted as a single symbol.
TheCoelacanth 5y ago
There's a simple test for readability. Someone familiar with the codebase reads the code and tries to understand what it does (a.k.a. a code review). If it takes a low effort, then it's high readability. If it takes a high effort, then it's low readability.

seadan83 5y ago

This is where I have found review to be really tricky. I have reviewed so much unreadable code that I lose faith sometimes. More to the point, readability issues are often perceived as nitpicks. Eventually after a few rounds of readability issues being addressed I can tell what the actual code is doing. At that point the major suggestions and some rework comes out, but now the author feels burned, nitpicked, and now instead of being done "their perfectly functional" code needs more work and the whole thing is not done. Do this a few times and relationships begin to break down. Hence, I now try really hard to ignore readability until the end. I don't know if this partly just hostile developers, different understanding of code. Ie, is code for humans to read, or for machines to execute? Or if it's dealing with feature factory developers that just want to keep adding despite how brittle the code is becoming. I presume the problem is somewhere in between, but to another extent the normalcy of the absurd where it is normal to spend 5 minutes per line of code to understand it, it is normal to havk in features and do manual regression testing, etc..