4 ms·

> Some hardward manufacturers only ship new hardware once a year. A line snuck in through their code review. One big issue that I'm not really seeing discusse

by YZF 7y ago

> Some hardward manufacturers only ship new hardware once a year. A line snuck in through their code review. One big issue that I'm not really seeing discussed is that in many cases code reviews end up being very subjective. "hard to maintain", "not readable", "brittle", "needs more tests" are all super subjective. There is no magic tool that can tell you the future maintenance costs (vs. the current costs), whether some area is more likely to see issues, etc. A lot of this is a matter of judgement. One old recommendation I recall is to limit the scope of code reviews to finding bugs. If there is a bug that's pretty objective, the code does not work.

bt848 7y ago
You can draw bright lines about some of these things. If a change claims to fix some bug, a test must demonstrate that. If I patch just the test into HEAD and run it, it should fail. If this is not the case then the change "needs more tests". "Not readable" is why Google also has the "readability" process. A person without readability needs the pre-submit approval of someone with readability in that language. After a while, new people become acculturated and gain readability. "Matter of judgement" is why hiring is absolutely critical to success in software. You must hire people with good judgement, and to the extent that you might intentional or unintentionally hire bozos, do so at a low enough rate that you can indoctrinate them.
- YZF 7y ago
  "every bug fix must be accompanied with a test that demonstrates the bug/fix" is just dogma IMNSHO. (also see "closing the barn doors after the horses have escaped" or "lightning doesn't strike twice at the same place") The typical rationale is the tests will now catch a re-introduction of the bug or regression. The reality is that often the tests bloat the code base, they cost in future maintenance making the code harder to refactor, they may introduce their own flakiness (the test itself might have bugs), and the bug that's fixed is generally unlikely to recur. You want good tests- not just a random collection of tests that reflect the history of bugs in the code. I often end up writing a test as part of trying to understand or reproduce the bug, and that test will often stay... But not always. Sometimes it's the right thing to do. Sometimes it's not. I.e. subjective. The lines aren't always so bright. To complicate things further it's very hard to gauge the effectiveness of various processes. Is following those specific guidelines a net positive or negative? who knows. I think there's consensus that code reviews in general are a good idea so probably just by having a second person read the code, have a discussion with the author, with no guidelines, you're getting most of the benefit (obvious issues surface, more than one person is familiar with the code etc.). My gut feel is that the rest is in the noise. I do agree hiring is critical ;) yet another subjective process there. Ideally you'd only hire people with a track record of producing good software in your area of business, most other indicators are probably pretty random. If you're looking to build a first person shooter you should hire John Carmack. Hope he likes your code review guidelines though. Another random side comment is that IMO design issues should probably be identified before code is written. EDIT: random semi-related trivia: https://kotaku.com/the-exceptional-beauty-of-doom-3s-source-code-5975610 https://kotaku.com/the-exceptional-beauty-of-doom-3s-source-...
  - bt848 7y ago
    https://mobile.twitter.com/dvyukov/status/1169544167871131653 https://mobile.twitter.com/dvyukov/status/116954416787113165... This tweet perfectly captures why all bug fixes must have tests. Linux has no tests and no testing culture and it is 17 million lines of juicy hot garbage. Google is mostly tests and it is one of the largest and most successful C++ projects in history of our industry. I feel justified in standing my ground when reviewing under-tested code.
    - YZF 7y ago
      > Google is mostly tests and it is one of the largest and most successful C++ projects in history of our industry. I'm not quite sure what you mean here? Are you talking about Search? Ads? Cloud? Self driving cars? Google has various products of varying business and technical success levels. Some stuff I feel is a good example of quality software (let's say Go for example or maybe Maps) and some stuff seems to just be very poor software (let's say Google Hangouts Chat or Google Sheets). Some stuff is really good business and some gets cancelled (with little apparent correlation to quality). Google is so rich that perhaps it is succeeding despite some practices and not because of them. It's initial success probably predates all these practices and there's plenty of quality software in the history of the industry that has been produced using a variety of other practices. Even if you are right and Google is the most successful C++ code base ever that's still not evidence that some particular practice is the cause of that. There is no way to measure any of this and "feeling" doesn't really cut it. Some practices apply more to certain kinds of software and possibly less to others. Doesn't Google use Linux? For juicy hot garbage it's done pretty well. With a budget that's a fraction of what Google has. It's also not C++. re-EDIT: Another thing to note is that Google has a large number of engineers developing internal tools, build systems, test engineers etc. With all the automation around dealing with flaky tests and testing in general at scale perhaps this works better for Google. Other companies who do not have the luxury of having 100's of engineers work on their build or test systems may run into different problems when they try to do things the Google way. Because Google is overall fairly opaque (the open bits are exceptions) it's hard to really come to a conclusion about how well they do software. I've heard different stories from different sources and I'll bet there's lots of internal variation as well. The success as a company is undeniable but there's more to that than software.
- foobarian 7y ago
  On judgment: Linus has a great way of looking at it as taste. There is a short segment in his TED talk with a nice example. https://www.youtube.com/watch?v=qrYt4bbEUrU https://www.youtube.com/watch?v=qrYt4bbEUrU starting at 14:20
- flukus 7y ago
  How would I go about writing a test for an extremely rare race condition between two components 10 levels of indirection away covering 10s of thousands of lines of code? Because I've fixed bugs like that but can't fathom a way to write a test for it, I only even found the cause by grepping 6 months worth of logs and finding a recurring pattern. I briefly considered writing a debugger script to pause at the right spots so QA could replicate the issue but would be a waste of everyone's time and I wouldn't want that in an automated suite.
  - bt848 7y ago
    There are tools for that stuff. You can run the unit tests a million times and count the failures for starters. You can also run tests under TSAN which often just points out races. You can also inject clocks or other dependencies and simply create the precondition necessary and advance time manually to show the problem.
cr1222 7y ago
Things like maintainability are hard to objectively define and measure, but still important. If you let issues pile up a medium-to-large team working on a codebase for a few years can turn it into something completely incomprehensible and and impossible to chain. I make a lot of comments along those lines when I review code and I think overall it's helped significantly with technical debt and it would be very limiting to not be able to use my judgement on these matters when reviewing code. I definitely always provide some explanation, e.g. some examples of why the test coverage is inadequate, or why the code seems more complex than necessary. But this still relies to a significant degree on the reviewer's intuition and judgement. I think it's reasonable to look at code and ask "will someone be able to understand this in a year's time?" or "how likely is someone to introduce a bug when changing this logic?". You won't get totally objective answers but I think you get some worthwhile insight. There are various measures you can look at as heuristics - cyclomatic complexity, that sort of thing. I think also requiring good test coverage steers things in the right direction since it's often easier to simplify the code than to achieve full coverage. The "finding bugs" criteria also doesn't really work if the code is so complex that a reviewer can't convince themselves of its correctness.
- YZF 7y ago
  I definitely agree with the overall sentiment; one needs to be constantly on guard to maintain a good code base. I don't think it's just a matter of code review though. A bad team will take this "how to do a code review" and still produce bad stuff. A good team probably doesn't need "how to do a code review". The best code reviews I've participated in were more informal affairs. They're generally in person discussion rather than the distant commenting on some review platform.

by YZF 7y ago

bt848 7y ago

You can draw bright lines about some of these things. If a change claims to fix some bug, a test must demonstrate that. If I patch just the test into HEAD and run it, it should fail. If this is not the case then the change "needs more tests". "Not readable" is why Google also has the "readability" process. A person without readability needs the pre-submit approval of someone with readability in that language. After a while, new people become acculturated and gain readability. "Matter of judgement" is why hiring is absolutely critical to success in software. You must hire people with good judgement, and to the extent that you might intentional or unintentionally hire bozos, do so at a low enough rate that you can indoctrinate them.

YZF 7y ago
"every bug fix must be accompanied with a test that demonstrates the bug/fix" is just dogma IMNSHO. (also see "closing the barn doors after the horses have escaped" or "lightning doesn't strike twice at the same place") The typical rationale is the tests will now catch a re-introduction of the bug or regression. The reality is that often the tests bloat the code base, they cost in future maintenance making the code harder to refactor, they may introduce their own flakiness (the test itself might have bugs), and the bug that's fixed is generally unlikely to recur. You want good tests- not just a random collection of tests that reflect the history of bugs in the code. I often end up writing a test as part of trying to understand or reproduce the bug, and that test will often stay... But not always. Sometimes it's the right thing to do. Sometimes it's not. I.e. subjective. The lines aren't always so bright. To complicate things further it's very hard to gauge the effectiveness of various processes. Is following those specific guidelines a net positive or negative? who knows. I think there's consensus that code reviews in general are a good idea so probably just by having a second person read the code, have a discussion with the author, with no guidelines, you're getting most of the benefit (obvious issues surface, more than one person is familiar with the code etc.). My gut feel is that the rest is in the noise. I do agree hiring is critical ;) yet another subjective process there. Ideally you'd only hire people with a track record of producing good software in your area of business, most other indicators are probably pretty random. If you're looking to build a first person shooter you should hire John Carmack. Hope he likes your code review guidelines though. Another random side comment is that IMO design issues should probably be identified before code is written. EDIT: random semi-related trivia: https://kotaku.com/the-exceptional-beauty-of-doom-3s-source-code-5975610 https://kotaku.com/the-exceptional-beauty-of-doom-3s-source-...
- bt848 7y ago
  https://mobile.twitter.com/dvyukov/status/1169544167871131653 https://mobile.twitter.com/dvyukov/status/116954416787113165... This tweet perfectly captures why all bug fixes must have tests. Linux has no tests and no testing culture and it is 17 million lines of juicy hot garbage. Google is mostly tests and it is one of the largest and most successful C++ projects in history of our industry. I feel justified in standing my ground when reviewing under-tested code.
  - YZF 7y ago
    > Google is mostly tests and it is one of the largest and most successful C++ projects in history of our industry. I'm not quite sure what you mean here? Are you talking about Search? Ads? Cloud? Self driving cars? Google has various products of varying business and technical success levels. Some stuff I feel is a good example of quality software (let's say Go for example or maybe Maps) and some stuff seems to just be very poor software (let's say Google Hangouts Chat or Google Sheets). Some stuff is really good business and some gets cancelled (with little apparent correlation to quality). Google is so rich that perhaps it is succeeding despite some practices and not because of them. It's initial success probably predates all these practices and there's plenty of quality software in the history of the industry that has been produced using a variety of other practices. Even if you are right and Google is the most successful C++ code base ever that's still not evidence that some particular practice is the cause of that. There is no way to measure any of this and "feeling" doesn't really cut it. Some practices apply more to certain kinds of software and possibly less to others. Doesn't Google use Linux? For juicy hot garbage it's done pretty well. With a budget that's a fraction of what Google has. It's also not C++. re-EDIT: Another thing to note is that Google has a large number of engineers developing internal tools, build systems, test engineers etc. With all the automation around dealing with flaky tests and testing in general at scale perhaps this works better for Google. Other companies who do not have the luxury of having 100's of engineers work on their build or test systems may run into different problems when they try to do things the Google way. Because Google is overall fairly opaque (the open bits are exceptions) it's hard to really come to a conclusion about how well they do software. I've heard different stories from different sources and I'll bet there's lots of internal variation as well. The success as a company is undeniable but there's more to that than software.
foobarian 7y ago
On judgment: Linus has a great way of looking at it as taste. There is a short segment in his TED talk with a nice example. https://www.youtube.com/watch?v=qrYt4bbEUrU https://www.youtube.com/watch?v=qrYt4bbEUrU starting at 14:20
flukus 7y ago
How would I go about writing a test for an extremely rare race condition between two components 10 levels of indirection away covering 10s of thousands of lines of code? Because I've fixed bugs like that but can't fathom a way to write a test for it, I only even found the cause by grepping 6 months worth of logs and finding a recurring pattern. I briefly considered writing a debugger script to pause at the right spots so QA could replicate the issue but would be a waste of everyone's time and I wouldn't want that in an automated suite.
- bt848 7y ago
  There are tools for that stuff. You can run the unit tests a million times and count the failures for starters. You can also run tests under TSAN which often just points out races. You can also inject clocks or other dependencies and simply create the precondition necessary and advance time manually to show the problem.

cr1222 7y ago

Things like maintainability are hard to objectively define and measure, but still important. If you let issues pile up a medium-to-large team working on a codebase for a few years can turn it into something completely incomprehensible and and impossible to chain. I make a lot of comments along those lines when I review code and I think overall it's helped significantly with technical debt and it would be very limiting to not be able to use my judgement on these matters when reviewing code. I definitely always provide some explanation, e.g. some examples of why the test coverage is inadequate, or why the code seems more complex than necessary. But this still relies to a significant degree on the reviewer's intuition and judgement. I think it's reasonable to look at code and ask "will someone be able to understand this in a year's time?" or "how likely is someone to introduce a bug when changing this logic?". You won't get totally objective answers but I think you get some worthwhile insight. There are various measures you can look at as heuristics - cyclomatic complexity, that sort of thing. I think also requiring good test coverage steers things in the right direction since it's often easier to simplify the code than to achieve full coverage. The "finding bugs" criteria also doesn't really work if the code is so complex that a reviewer can't convince themselves of its correctness.

YZF 7y ago
I definitely agree with the overall sentiment; one needs to be constantly on guard to maintain a good code base. I don't think it's just a matter of code review though. A bad team will take this "how to do a code review" and still produce bad stuff. A good team probably doesn't need "how to do a code review". The best code reviews I've participated in were more informal affairs. They're generally in person discussion rather than the distant commenting on some review platform.