Code Reviews.
-
I agree, except where the error is Ironic, Amusing or of such vast incomprehensibleness as to render the reader speechless and/or catatonic.
--------------------------------- I will never again mention that I was the poster of the One Millionth Lounge Post, nor that it was complete drivel. Dalek Dave CCC Link[^]
-
How do you guys do them?
cheers, Chris Maunder The Code Project | Co-founder Microsoft C++ MVP
Not sure if you are familiar, but in TFS2012 there is some really cool options. Basically when a junior grunt needs to check something in, he requests a code review (then he goes to the bathroom or something). I get an alert, when I have time (more often I'd rather just get it over), I click "Yes, I accept your code review request". I then look at his changes to the server version, adding comments to code when needed (Now this is where TFS2012 is awesome, the comments don't actually go into the code, but into the code review and have a corresponding section of code, super cool). I typically don't review it in front of him, because most of the time I don't need to. I also review each check in to any shared code, so it happens often. If it's his own project, I'll review periodically to see if the architecture is FUBARed so he doesn't waste time going down a bad path. Now, sometimes, if the review to shared code requires a complex explanation, I call him over to my desk and apply proper punishment show him the errors of his ways, all the while adding comments as I bring up my points. Then I send it back to him to try again.
-
If you're even half good, you place a hand on the machine and feel the code...
Reality is an illusion caused by a lack of alcohol
Sounds like this story could get dirty. :-D
-
How do you guys do them?
cheers, Chris Maunder The Code Project | Co-founder Microsoft C++ MVP
Since I work alone, my code reviews are pretty simple. If it's been less than 1 day since I wrote the code, I think it is the most brilliant code ever written. If it's been more than a day, then I write comments about the author have been insane and it all needs a rewrite whenever there is time - which there never is.
CQ de W5ALT
Walt Fair, Jr., P. E. Comport Computing Specializing in Technical Engineering Software
-
I've still got my dog, so I'm alright. He's just as reliable as he ever was.[^] :-\
That is the first time I have ever upvoted something over two years old...
If you get an email telling you that you can catch Swine Flu from tinned pork then just delete it. It's Spam.
-
Manifesto: stop the posting of unhelpful comments pointing out that someone has "got something wrong" where the "something wrong" part is relatively minor, thus causing distraction. Such examples examples include spelling/syntactical/grammatical mistakes where is it pretty easy to work the originator's intention. Why "unhelpful comment"? At best the comment make the poster feel superior to someone else whilst denigrating or undermining the original poster especially when the originator is trying to add something to the debate, at worst it diverts the discussion away from the topic. Some people seem to mistake the Internet for an academic paper. -------------------- If you agree with the sentiment, please feel free to copy and paste this manifesto into your replies, including this comment -------------------- In this case, I misread CM's question as "Do you Guys do them"
Sort of a cross between Lawrence of Arabia and Dilbert.[^]
-Or-
A Dead ringer for Kate Winslett[^]Keith Barrow wrote:
Some people seem to mistake the Internet for an academic paper
...and insist that informal discussions remain "on topic". ;) If you don't appreciate a reply, my advice it to just leave it without a further reply and let that branch die.
-
Keith Barrow wrote:
Some people seem to mistake the Internet for an academic paper
...and insist that informal discussions remain "on topic". ;) If you don't appreciate a reply, my advice it to just leave it without a further reply and let that branch die.
AspDotNetDev wrote:
...and insist that informal discussions remain "on topic
Nope, just want to reduce noise.
Sort of a cross between Lawrence of Arabia and Dilbert.[^]
-Or-
A Dead ringer for Kate Winslett[^] -
How do you guys do them?
cheers, Chris Maunder The Code Project | Co-founder Microsoft C++ MVP
I give my coworkers code a quick glance over when I pull it from source control and send an email if something looks odd. AFAIK I'm the only person who does this. Everyone always agrees when the subject is raised that it's a 'good idea' and if there was 'more money in the budget/time in the schedule' we should start doing them. 'Maybe on the next funding increment.' Which is why I do what I do. :sigh: :sigh:
Did you ever see history portrayed as an old man with a wise brow and pulseless heart, waging all things in the balance of reason? Is not rather the genius of history like an eternal, imploring maiden, full of fire, with a burning heart and flaming soul, humanly warm and humanly beautiful? --Zachris Topelius Training a telescope on one’s own belly button will only reveal lint. You like that? You go right on staring at it. I prefer looking at galaxies. -- Sarah Hoyt
-
How do you guys do them?
cheers, Chris Maunder The Code Project | Co-founder Microsoft C++ MVP
We have developed an in-house tool that keeps track of all the changes to the source code. Then all developers have a mandatory review day, once every two weeks (not on the same day). For each review, there is a check list of things to go through. We are encouraged to review code for an area that we are at least a bit familiar with so that we can better assess the impact of those changes on the rest of the functionality. The review results are submitted via the same tool and the developer can make the suggested changes or reply with an explanation of why those changes are not necessary/helpful/whatever. One of our biggest problems, though, is that the reviewers differ widely in opinion sometimes and sometimes some of them can be quite pedantic.
-
How do you guys do them?
cheers, Chris Maunder The Code Project | Co-founder Microsoft C++ MVP
We use ReviewBoard to post code reviews from git diffs. The RB link is then sent out to relevant people in the group for review and sign-off before pushing the code to 'central' repository. When the diff is being 'posted' to RB, the script that does the job amends the commit message to include the RB link to which the diff was sent for review and hence each commit pushed would ideally have an RB link where you can see who approved the change.
a.k.a. Vite Phoenix and Vite Zeus... Proud member and co-founder of OlympianZ
-
How do you guys do them?
cheers, Chris Maunder The Code Project | Co-founder Microsoft C++ MVP
Well actually, we do it ourselves in peer review sessions with lots of manic laughters, booze, mantras and faith.
-
If you're even half good, you place a hand on the machine and feel the code...
Reality is an illusion caused by a lack of alcohol
You are always right. If everyone used the force then they would be right as well. I often close my eyes when coding. The force does work Nagy Vilmos. Just wish that the code reviewers and the network pigs would do the same.....
"Rock journalism is people who can't write interviewing people who can't talk for people who can't read." Frank Zappa 1980
-
How do you guys do them?
cheers, Chris Maunder The Code Project | Co-founder Microsoft C++ MVP
One Cardinal Rule: The code doesn't change unless you can convince the author that the change is necessary or useful. This really sets the tone for the code review, it prevents "religious wars" and makes sure that the review is all about understanding the reason why someone chose to do things this way.
-
How do you guys do them?
cheers, Chris Maunder The Code Project | Co-founder Microsoft C++ MVP
All of the software my department writes is in-house apps used only be our lines of business, or web apps used by our clients. Nothing we do is shipped. We use Visual Studio's static code analyis for the simple stuff. We have some items that are globally suppressed (such as null parameter checking), and temporary local suppressions for other items. With exceptions only for platform ports, temporary suppressions are actually temporary; we GREP them out of the code before it gets to any end user testing. With a platform port (which we're pushing now because some of our stuff, even though working, is *very* old, and we can't get support for the tech stack), especially those that are coming from "legacy" code, i.e. with no automated tests, the algorithms are retained until such time as tests can be wrapped around them to verify functionality. Even those seldom go to production, though, and when they do, the very next iteration for that app is likely to get them stripped out. We use NCover, and lately OpenCover, to automate the test coverage. We're aiming for 80% right now, and are at near 90% for the greenfield and completely ported stuff, though overall, because of the "legacy" code, we're still in the 40% range. We've been using the Redgate stuff for performance monitoring and memory management.
Currently reading: "Gateway", by Frederik Pohl
-
How do you guys do them?
cheers, Chris Maunder The Code Project | Co-founder Microsoft C++ MVP
Reviews are mandatory for all software releases. Before a sprint can be signed off on, or the code even go to test, the code must go through a peer code review of at least 2 engineers (none of which are the author) and a QA member. To do this, we use Crucible, which we have linked in with SVN and Jira. I haven't been involved with a sit down code review for over 3 years.
-
How do you guys do them?
cheers, Chris Maunder The Code Project | Co-founder Microsoft C++ MVP
We do them for all checked in code. Just grab a developer that is familiar with the area, then go through all the modifications line by line. Then, the check in is marked with the name of the coder and the reviewer(s). It really has to be kept light hearted and simple to avoid hard feelings. The biggest issue, as someone else has commented, is getting the reviewer to understand the bigger picture of what you're doing so that they can fully understand your changes, otherwise the review would be fairly superficial. I can say that quite a few blunders have been prevented by the process so I would say that it is a very worthwhile exercise. Just avoid turning the process into a formality.
-
How do you guys do them?
cheers, Chris Maunder The Code Project | Co-founder Microsoft C++ MVP
The company I work for is a startup, so there are only two coders: my boss and me (I'm still quite junior). As such, our process is rather informal: he looks at my code and tells me 1.) where I screwed up (bug-causing stuff and/or bad practices) and 2.) what's not really wrong, but not how he would have done it (stylistic things taking a slight backseat to performance/readability issues, but they're still discussed, as our style guide is what he says it is). He basically gives me a project, I can ask questions as I go, and then when I'm done, we go over it sometime before we get into acceptance testing so I can address whatever points he's made. Hopefully the advanced nature of this process isn't too overwhelming. :)
-
How do you guys do them?
cheers, Chris Maunder The Code Project | Co-founder Microsoft C++ MVP
Code Reviews or Code Inspections (all the same), depending on how you do them. Also, it depends on how formal you want to conduct them, if you need to document them for your company or some approval agency. There are also several good tools out there, some of the other posters have mentioned. We use Code Collaborator, which is internet based, creates good records and reports, but not good for full code context. If you want to look for articles on code reviews, there are quite a few, dating back to the 70s and work done at IBM. (Back then, when there were 300 guys on a project, code reviews were much easier to get done. Today, with few people or only one person on a project, it is difficult to review the context of the code and design.) Since I did my MBA thesis on code reviews and how they help increase the quality of the product (quantifiable), one author comes to mind: M. E. Fagan, from IBM. Those guys from IBM had it down to a science. Even documented how code review meetings were to be run and the ins and outs of code inspections. Give them a look up on the web.
-
We use ReviewBoard to post code reviews from git diffs. The RB link is then sent out to relevant people in the group for review and sign-off before pushing the code to 'central' repository. When the diff is being 'posted' to RB, the script that does the job amends the commit message to include the RB link to which the diff was sent for review and hence each commit pushed would ideally have an RB link where you can see who approved the change.
a.k.a. Vite Phoenix and Vite Zeus... Proud member and co-founder of OlympianZ
+1 for ReviewBoard. I use RB at Apache and it's the best code diff tool I've ever seen. There's an interactive comment feature and it really facilitates code review nicely. The changes are put into context and reviewers don't have to go through huge mental gymnastics to understand the proposed diffs. That said, if I post a RB entry and nobody responds to it then have I had a code review or not? If there's no replies after a week or so then that signals approval in my mind. I create ReviewBoard entries sometimes just so see what my changes look like and then discard them. If a commit will change 40 or so files I like to make sure that no junky debug edits will part of my check in.
They will never have seen anything like us them there. - M. Spirito
-
+1 for ReviewBoard. I use RB at Apache and it's the best code diff tool I've ever seen. There's an interactive comment feature and it really facilitates code review nicely. The changes are put into context and reviewers don't have to go through huge mental gymnastics to understand the proposed diffs. That said, if I post a RB entry and nobody responds to it then have I had a code review or not? If there's no replies after a week or so then that signals approval in my mind. I create ReviewBoard entries sometimes just so see what my changes look like and then discard them. If a commit will change 40 or so files I like to make sure that no junky debug edits will part of my check in.
They will never have seen anything like us them there. - M. Spirito
Mine is being used in a big company and we impose the necessity of having to do code review before pushing our code. So if we don't hear from those whom we have included in the CR (code review), we nag them till they do so or notify others that you're stuck because you haven't received any code review on the changes you've made.
a.k.a. Vite Phoenix and Vite Zeus... Proud member and co-founder of OlympianZ
