Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Using checklists for code review

blog.rbcommons.com

Post reply on HN
Print view View original

21–26 of 26 posts

Re: Using checklists for code review

#21

Post by zimpenfish »

mseebach wrote: post #10

It sounds minor, but I think it's pretty important: The points on his checklist aren't clear yes/no questions, they're a bunch of things you should remember to consider. Specifically, how do you check "Correctness of algorithms"? What is the criteria to pass that? "Well, I think it's right", of course you do, otherwise you wouldn't have committed it. Formal proof? Unit tests? Items phrased like this are ripe to becom…

I think even a woolly item like "Correctness of algorithms" is helpful - "why is he using bubblesort for umpzillion items?"

It's not really meant as a yes/no; it's meant to prompt the code reviewer (not you!) into checking the algorithms for insanity. Easy to forget that when you're rushed and hassled and wanting your lunch.

Also, aren't the majority of flight checklists for abnormal situations? And specifically to make sure that things are handled in a sane and sensible fashion when everything is going off klaxon-wise and/or on fire?

Re: Using checklists for code review

#22

Post by swah »

npsimons wrote: post #2

Some of these can be caught automatically, and the more automation (taking the human out of the loop), the better, because humans can gloss over a checklist or accidentally skip something. My current project uses a combination of Gerrit, Jenkins and Sonar, along with unit/regression tests, so that every single change is checked automatically to make sure it builds and passes the tests, along with requiring at least o…

Side discussion: why do you guys think software like Gerrit, Jenkins and Sonar, software that is used by many folks and mostly praised, is written in Java (instead of say, Python)?

Re: Using checklists for code review

#23

Post by npsimons »

swah wrote: post #22
npsimons wrote: post #2

Some of these can be caught automatically, and the more automation (taking the human out of the loop), the better, because humans can gloss over a checklist or accidentally skip something. My current project uses a combination of Gerrit, Jenkins and Sonar, along with unit/regression tests, so that every single change is checked automatically to make sure it builds and passes the tests, along with requiring at least o…

Side discussion: why do you guys think software like Gerrit, Jenkins and Sonar, software that is used by many folks and mostly praised, is written in Java (instead of say, Python)?

Honestly? Don't know, don't care. And yes, we have arguments about the pros and cons of programming languages all the time. I'm guessing that Java started down this road of appearing more "professional", and that went some ways towards people actually wanting to prove it, so things like accountability and reproducibility started getting serious consideration in mainstream programming. And it's not like everything is Java; git, vera, cpplint, valgrind are all not written in Java.

To be honest, the only reason we are using C++ is because it's mandated by the project, and quite frankly, it's what we're good at (we'd probably be working another C++ project if this one wasn't in C++). And yes, we know C++ is a kludge; quite frankly, I wouldn't trust a C++ "expert" who didn't question design decisions of C++.

Re: Using checklists for code review

#24

Post by npsimons »

joelhooks wrote: post #14

Checklists are OK, but beware the checklist. They tend to cause complacency. "It wasn't in the checklist!"

"It wasn't in the checklist!"

"Well now it is, and there's a unit test for it, and you won't be able to commit code that breaks that unit test. Problem solved."

Re: Using checklists for code review

#25

Post by mseebach »

zimpenfish wrote: post #21
mseebach wrote: post #10

It sounds minor, but I think it's pretty important: The points on his checklist aren't clear yes/no questions, they're a bunch of things you should remember to consider. Specifically, how do you check "Correctness of algorithms"? What is the criteria to pass that? "Well, I think it's right", of course you do, otherwise you wouldn't have committed it. Formal proof? Unit tests? Items phrased like this are ripe to becom…

I think even a woolly item like "Correctness of algorithms" is helpful - "why is he using bubblesort for umpzillion items?" It's not really meant as a yes/no; it's meant to prompt the code reviewer (not you!) into checking the algorithms for insanity. Easy to forget that when you're rushed and hassled and wanting your lunch. Also, aren't the majority of flight checklists for abnormal situations? And specifically to m…

The developer should absolutely follow the checklist as well, otherwise he's wasting the reviewers time.

So, what you're describing is actually a good yes/no question: "Is the best algorithm for the problem used?" (or, better: "Has functionality that is available in a library been implemented by hand?", but that's not your point :) )

No, flights use checklists for even completely routine thing (especially for routine things, because that's where sloppiness will manifest itself first). For an example, if you forget to set flaps correctly before you hit the throttle for take off, you're going to have a bad time, so that's on the checklist. (Of course, these things are largely automated now, so modern aircrafts will yell at you if you don't. I think Airbuses will even override the throttle)

Re: Using checklists for code review

#26

Post by eru »

mseebach wrote: post #25
zimpenfish wrote: post #21

Earlier quoted context omitted.

I think even a woolly item like "Correctness of algorithms" is helpful - "why is he using bubblesort for umpzillion items?" It's not really meant as a yes/no; it's meant to prompt the code reviewer (not you!) into checking the algorithms for insanity. Easy to forget that when you're rushed and hassled and wanting your lunch. Also, aren't the majority of flight checklists for abnormal situations? And specifically to m…

The developer should absolutely follow the checklist as well, otherwise he's wasting the reviewers time. So, what you're describing is actually a good yes/no question: "Is the best algorithm for the problem used?" (or, better: "Has functionality that is available in a library been implemented by hand?", but that's not your point :) ) No, flights use checklists for even completely routine thing (especially for routine…

> "Is the best algorithm for the problem used?"

Rather: "Is the algorithm good enough?"

Post reply on HN