3 ms·

Code review works. Ok, yeah I'd say that is the one practice which I've seen consistently tied to high(er) quality work. Article is pretty handwavy about epist

by m3047 3y ago

Code review works. Ok, yeah I'd say that is the one practice which I've seen consistently tied to high(er) quality work. Article is pretty handwavy about epistemology and just things generally. But I'd say not all code reviews are equal, and that the best ones aren't about the code (in the way that baseball is not about bats and balls) and are instead about people learning to talk to each other, about code. Probably also about how that "campfire" provides a venue to discuss other institutional lore and hence culture.

majormajor 3y ago
There's no better example of the sort of problems "you get what you measure" causes than how even code review can go wrong. I've seen people use code review as an excuse not to test their own code, which is one of those things that made my eyes pop out the first couple of times I looked at a request with code that blatantly didn't compile or run. That's not to say "don't do code review" but it's worth making sure the team understands the goals and their individual responsibilities vs just blindly putting a process out there. Like you say, "communication" is handwavy but critical.
kqr 3y ago
Code review really does work, yes. Boehm has it at 60 % defects discovered at 15 % additional effort. Apparently "directed" or "scenario-based" code review works even better, uncovering an additional 20 or so percentage points of defects. But I have yet to find out what that means! Does it mean having a list of common problems and looking for one at a time? Does it mean taking a very concrete user story and mentally executing the code involved in satisfying it?
- m3047 3y ago
  Best code reviews I've participated in were in a CMM 3 shop which was a division of a large corp. There was overarching method to the madness, but as a lowly engineer serving our manufacturing customer it looked more or less like: 1) pair up (from my POV) randomly (not everybody included in this is a coder) 2) (from my POV) flip a coin to decide who picks an item to review 3) pick an item you've touched recently or if there isn't one something you're going to touch in the next week (could be doc, build scripts, other things depending on what your job is; you might adjust your selection depending on your peer's duties and responsibilites, no sense having them review something they will never ever even look at). 4) flip a coin to decide who is going to explain what the code does 5) one person explains what the code does / doc means to the other person; the other person asks clarifying questions Repeat the above steps weekly. There was a sheet that was filled out that recorded defects found in categories such as logic, (failure of) abstraction, requirements, doc. We talked about style but there wasn't really an enforced style guide. It can be uncomfortable at first to talk about somebody else's code or have them explain your code. You learn to listen and have a sense of humor and be patient. Perhaps the most poignant lessons learned had to do with doc in code (generally more is better but watch out for zombie comments) and sometimes the most clever way of doing something isn't worth the potential for misunderstandings. There was usually something which needed more doc! Improving tests was also a common outcome. We did find logic errors occasionally, which would inevitably lead to discussion around improving tests. Requirements issues and abstraction / architecture issues came up infrequently and typically spawned email or a meeting which came later.
  - kqr 3y ago
    This sounds interesting! So instead of someone else asynchronously asking clarifying questions of the author, one person synchronously asks clarifying questions of someone who is not the author? I really like the idea that the artifact should be understandable to people who don't have immediate access to the author. What was the process if the artifact was "rejected" in this type of review and neither half of the pair had enough understanding to fix it?
    - m3047 3y ago
      Good questions! > instead of ... asynchronously Yes. > What was the process if the artifact was "rejected" We only did it once a week and all code was not reviewed. Well, I should say not all code was formally reviewed. And we had tests, and some of them came from "real" Engineering (level 1). We didn't have 100% coverage, but "more tests" was a frequent outcome, and it was implicit that this wasn't limited to the specific artifacts which were reviewed. There might be a timing or integrity issue and so there might be some attempt to determine what the appropriate scope of testing was (the emails / meetings I mentioned). I don't recall that anything was ever rejected in one of these. [Edit - added] This was mostly front-loaded, because we never wrote code until there was some consensus on what change was needed. So some of these were somebody bringing a "problem child" and either walking through it or enjoying the entertainment as some rando did it. The process existed with the implicit goal of levelling everybody into some shared framework of discussing code. We would reach out to peers all the time for informal review and it was pretty frictionless (email somebody with a branch and particular range of LOC). About the only time you wouldn't reach out like that was if you had pretty close to 100% test coverage. 8-) > neither ... of the pair had enough understanding to fix it In another fine example of "moral hazard for good" that never happened! Less facetiously those ended up being the requirements / architecture issues. So as far as the process was concerned the pair reached a consensus. > synchronously asks clarifying questions of someone who is not the author Yes, about half the time the person explaining the code was the one who'd never seen it before that moment. LOL. Brutal. OTOH I don't know if it's more brutal than explaining code to someone whose job doesn't actually include coding... Just different. Like I said, it improves people's ability to talk about code and creates a culture that makes it easier to seek help the rest of the time.

by m3047 3y ago

majormajor 3y ago

There's no better example of the sort of problems "you get what you measure" causes than how even code review can go wrong. I've seen people use code review as an excuse not to test their own code, which is one of those things that made my eyes pop out the first couple of times I looked at a request with code that blatantly didn't compile or run. That's not to say "don't do code review" but it's worth making sure the team understands the goals and their individual responsibilities vs just blindly putting a process out there. Like you say, "communication" is handwavy but critical.

kqr 3y ago

Code review really does work, yes. Boehm has it at 60 % defects discovered at 15 % additional effort. Apparently "directed" or "scenario-based" code review works even better, uncovering an additional 20 or so percentage points of defects. But I have yet to find out what that means! Does it mean having a list of common problems and looking for one at a time? Does it mean taking a very concrete user story and mentally executing the code involved in satisfying it?

m3047 3y ago
Best code reviews I've participated in were in a CMM 3 shop which was a division of a large corp. There was overarching method to the madness, but as a lowly engineer serving our manufacturing customer it looked more or less like: 1) pair up (from my POV) randomly (not everybody included in this is a coder) 2) (from my POV) flip a coin to decide who picks an item to review 3) pick an item you've touched recently or if there isn't one something you're going to touch in the next week (could be doc, build scripts, other things depending on what your job is; you might adjust your selection depending on your peer's duties and responsibilites, no sense having them review something they will never ever even look at). 4) flip a coin to decide who is going to explain what the code does 5) one person explains what the code does / doc means to the other person; the other person asks clarifying questions Repeat the above steps weekly. There was a sheet that was filled out that recorded defects found in categories such as logic, (failure of) abstraction, requirements, doc. We talked about style but there wasn't really an enforced style guide. It can be uncomfortable at first to talk about somebody else's code or have them explain your code. You learn to listen and have a sense of humor and be patient. Perhaps the most poignant lessons learned had to do with doc in code (generally more is better but watch out for zombie comments) and sometimes the most clever way of doing something isn't worth the potential for misunderstandings. There was usually something which needed more doc! Improving tests was also a common outcome. We did find logic errors occasionally, which would inevitably lead to discussion around improving tests. Requirements issues and abstraction / architecture issues came up infrequently and typically spawned email or a meeting which came later.
- kqr 3y ago
  This sounds interesting! So instead of someone else asynchronously asking clarifying questions of the author, one person synchronously asks clarifying questions of someone who is not the author? I really like the idea that the artifact should be understandable to people who don't have immediate access to the author. What was the process if the artifact was "rejected" in this type of review and neither half of the pair had enough understanding to fix it?
  - m3047 3y ago
    Good questions! > instead of ... asynchronously Yes. > What was the process if the artifact was "rejected" We only did it once a week and all code was not reviewed. Well, I should say not all code was formally reviewed. And we had tests, and some of them came from "real" Engineering (level 1). We didn't have 100% coverage, but "more tests" was a frequent outcome, and it was implicit that this wasn't limited to the specific artifacts which were reviewed. There might be a timing or integrity issue and so there might be some attempt to determine what the appropriate scope of testing was (the emails / meetings I mentioned). I don't recall that anything was ever rejected in one of these. [Edit - added] This was mostly front-loaded, because we never wrote code until there was some consensus on what change was needed. So some of these were somebody bringing a "problem child" and either walking through it or enjoying the entertainment as some rando did it. The process existed with the implicit goal of levelling everybody into some shared framework of discussing code. We would reach out to peers all the time for informal review and it was pretty frictionless (email somebody with a branch and particular range of LOC). About the only time you wouldn't reach out like that was if you had pretty close to 100% test coverage. 8-) > neither ... of the pair had enough understanding to fix it In another fine example of "moral hazard for good" that never happened! Less facetiously those ended up being the requirements / architecture issues. So as far as the process was concerned the pair reached a consensus. > synchronously asks clarifying questions of someone who is not the author Yes, about half the time the person explaining the code was the one who'd never seen it before that moment. LOL. Brutal. OTOH I don't know if it's more brutal than explaining code to someone whose job doesn't actually include coding... Just different. Like I said, it improves people's ability to talk about code and creates a culture that makes it easier to seek help the rest of the time.