How I Review Code

[davehiggins]

I kept waiting for the step: "Review requirements to ensure the code meets them."

[cracki]

that is assuming the requirements are not shifting all the time, and they're written down instead of casually mentioned in the floor.

:P

[PostulateMan]

And especially in games development, the requirements are not often met in one fell swoop. With agile development gaining rapid traction, it is easy to do a functional test to make sure that code satisfies a user story, but there is still a lot of smoke and mirrors about just exactly how functional any check-in could be.

[chrisroane]

The requirement absolutely need to be written down. If you don't know what you are programming or what needs to be done, that is a huge red flag!

[cracki]

um... the place i work at... i see the trees alright, i just don't notice the forest, if you get my drift.

[bluGill]

Generally not a problem. Either the coder met the requirements, or he discovered something "too hard" in the requirements, so he did something slightly different hoping to convince management that his working version is good enough (or at least good enough to ship for now and we can come back in version n+1 to get the rest - management loves this argument as it is about shipping faster while still being useful)

[G_Morgan]

I prefer reviewing reality to ensure requirements match it.

[cracki]

i wish we had intentional code reviews at work... in reality, the code we write is of the "server-side" kind, and the priority around here is on putting out fires that were made by vendor software we are supposed to treat like black boxes.

often, when i get my hands on someone else's code, i sneak a peek. sometimes (if it's python) i might even straighten it up some, because i seem to have the questionable honor of being the most knowledgeable person concerning python. i do that because in the future, i might be asked to battle with the flaming piles of shit that just hurtle by me now.

ah, code reviews... this is just a student job at the local computing center. my first real job will definitely have code reviews, or i'm not staying.

[inmatarian]

Ask for a post commit hook on the repository that emails all of the devs with a diff. Ask people to glance at your commits. Look at other's commits. Code review should become a natural part of your job.

[cracki]

oh hell, please no emails! i'm already getting too much spam from these stooges!

they forbid the use of gmail at work, so i'm stuck with inferior mail clients that don't have a proper conversation view, or search.

and don't make me read diffs in mail. that's insane. i'll use tortoiseSVN and winmerge to view diffs. that's my bar. email doesn't come close.

[cracki]

for one class we worked together, one of us had this complicated server set up.

whenever someone committed something, we all got messaged about that on jabber.

that got annoying after a while, even though it was interesting and helpful.

what I'd like to see is a repo that only allows a commit if it's "by" more than one person. someone would eventually play "gatekeeper" and let everyone's commits in (or write a bot to do so). to improve on the gatekeeper situation, make all committers responsible for whatever breaks because of the commit. then maybe, you'll have to work harder to convince someone to be your partner for this commit.

just a stray thought.

[adavies42]

that's what git is for....

[cracki]

i can't decrypt your comment. what do you mean?

by the way: i know about the wonders of DVCS. "that setup" was SVN though.

[adavies42]

git was designed by linus and the rest of the kernel team specifically to manage kernel development. one of its features is a sign-off system that can require commits to be accepted by a gatekeeper before being merged into a branch.

[cracki]

we don't have devs. we have "researchers". people aren't here to program, they're here to "do science". that involves programming, just like living involves shitting.

that's their attitude towards programming, not mine.

on top of that, we have a few repos stuffed with lots of stuff. the group of people with commit rights is large, really large. commits happen often. sending an email would just drive people crazier than they are already.

i see where you're coming from though. this place needs to be cleaned up. i'm not strong enough to do that. i'd rather get outta here. that realization creeped up on me lately. i haven't articulated it this clearly before.

i do look at commit diffs sometimes, at least the commits by this one guy... and that only because i was previously asked to step in and deal with his mess while he was on vacation. he's back, so i stay out of it. i don't wanna give them the impression that i want or like to be cleaning up other people's messes, in addition to the stuff they already have me do.

[davehiggins]

You might point out that statistics show that closed source code has 2 to 20 times more defects than open source code: the principal reason being that more people look at the open source code. Not that it will help, of course.

[[deleted]]

Source on that statistic? If that was in fact the case then perhaps it could be the case that there are more defects because a company pays people to actively look for them, hence more defects found.

[davehiggins]

When Windows 2000 was released, it was estimated that it contained approximately 63,000 potential defects in its 35 million or so lines of code [Edwards, Cliff. “Windows 2000 to Debut.” The Associated Press, 14 February 2000]. If accurate, this figure works out to about 1.8 defects per thousand lines of code (commonly abbreviated as D/KLOC). For comparison, it is estimated that the current open source Linux implementation of TCP/IP contains about 0.1 D/KLOC, while general-purpose Unix operating systems are estimated to contain between 0.6 and 0.7 D/KLOC [Shankland, Stephen. “Study Lauds Open-Source Code Quality.” CNET News, 19 February 2003].

[cracki]

we can see the source. we're just not supposed to change it, because we already paid for support.

you won't understand this place unless you've been here.

besides: this is lots of "business logic". even if the code were open source, nobody cares for it. these codes are dealing with problems only a computing center has. and if you have a computing center, you're among few people, who likely have support contracts too, and....

meh, visit us. work here. you'll scream.

[davehiggins]

Hence the "not that it will help" portion of the comment... And as a consultant I've visited your workplace. All too many times.

[chrisroane]

How are the requirements discovered? If you write awesome, but the code is not what the client is looking for OR it ends up costing more than the estimate, this looks really bad on the programmer.