Is the Code Review Parallel Programming or Singleton?
-
I have a new team forming up and we want to decide on the best way to formalize code reviews. Is the Code Review the responsibility of one person or the whole team? Is each, or certain, team member(s) allowed to review others' code or is there only one person who does the reviews? If it's only one person, then who reviews his/her code? I've heard of both in practice and some of the teams where I'm at use both patterns. I'm just looking for insight as to what everyone thinks is the best way to go and why.
- Arcond
Usually, you review each other's code. If your application processes highly sensitive data, it might be a good idea to hire consultants that specialize in code security.
-- Kein Mitleid Für Die Mehrheit
-
I think it should be responsibility of one person/team dedicated for the task - for better results - if you need to ship your code ASAP you are not going to sort out the mess in other's even if boss says so, on being pressed you will complete the formality and move on. Others can point out the issue if they found any anonymously - until unless direct intervention is required-, this will make team go smooth - my code is my pride-. Edit--- Looks like there is some issue with signature rendering so removed it from the post.
Amar Chaudhary wrote:
you are not going to sort out the mess in other's even if boss says so, on being pressed you will complete the formality and move on.
Are you saying that if you were aware of a flaw in a colleagues code, you would not mention it?
Henry Minute Do not read medical books! You could die of a misprint. - Mark Twain Girl: (staring) "Why do you need an icy cucumber?" “I want to report a fraud. The government is lying to us all.” I wouldn't let CG touch my Abacus! When you're wrestling a gorilla, you don't stop when you're tired, you stop when the gorilla is.
-
I have a new team forming up and we want to decide on the best way to formalize code reviews. Is the Code Review the responsibility of one person or the whole team? Is each, or certain, team member(s) allowed to review others' code or is there only one person who does the reviews? If it's only one person, then who reviews his/her code? I've heard of both in practice and some of the teams where I'm at use both patterns. I'm just looking for insight as to what everyone thinks is the best way to go and why.
- Arcond
IMHO everyone should review everyone's code; it is more in the spirit of the team - when reviewing someone's code it isn't a 'bad' thing to find issues, it's a positive thing. less experienced programmers get to see other's code more frequently than otherwise, and having to review it makes them take the time to understand and learn more experienced programmers are sometimes a little, well, set in their ways. Having others review their code is sometimes a learning experience for them, too any developer may look at someone's code and ask themselves "I wonder why they did it this way rather than that way?" If the question is asked, at least one of the participating parties learns something standards are a fluid thing - so multiple code reviewers allow discussions (aka raging flame-wars) to take place regarding standards and, hopefully, consensus can be reached Having one person do the reviews is prone to problems - the person leaves/is off sick/ is a loony/ with one person doing reviews, who reviews their code?
MVVM# - See how I did MVVM my way ___________________________________________ Man, you're a god. - walterhevedeich 26/05/2011 .\\axxx (That's an 'M')
-
Amar Chaudhary wrote:
you are not going to sort out the mess in other's even if boss says so, on being pressed you will complete the formality and move on.
Are you saying that if you were aware of a flaw in a colleagues code, you would not mention it?
Henry Minute Do not read medical books! You could die of a misprint. - Mark Twain Girl: (staring) "Why do you need an icy cucumber?" “I want to report a fraud. The government is lying to us all.” I wouldn't let CG touch my Abacus! When you're wrestling a gorilla, you don't stop when you're tired, you stop when the gorilla is.
No it means : when you have lack of time then you are not going to invest it in finding flaws of others code.
-
I have a new team forming up and we want to decide on the best way to formalize code reviews. Is the Code Review the responsibility of one person or the whole team? Is each, or certain, team member(s) allowed to review others' code or is there only one person who does the reviews? If it's only one person, then who reviews his/her code? I've heard of both in practice and some of the teams where I'm at use both patterns. I'm just looking for insight as to what everyone thinks is the best way to go and why.
- Arcond
Are you sure you mean parallel programming? Sounds more like pair programming, to me. Does someone not think that "pair" is a snazzy enough word?
I wanna be a eunuchs developer! Pass me a bread knife!
-
No it means : when you have lack of time then you are not going to invest it in finding flaws of others code.
If you worked for me and failed to participate fully in a procedure that had been set out, you wouldn't work for me.
Henry Minute Do not read medical books! You could die of a misprint. - Mark Twain Girl: (staring) "Why do you need an icy cucumber?" “I want to report a fraud. The government is lying to us all.” I wouldn't let CG touch my Abacus! When you're wrestling a gorilla, you don't stop when you're tired, you stop when the gorilla is.
-
If you worked for me and failed to participate fully in a procedure that had been set out, you wouldn't work for me.
Henry Minute Do not read medical books! You could die of a misprint. - Mark Twain Girl: (staring) "Why do you need an icy cucumber?" “I want to report a fraud. The government is lying to us all.” I wouldn't let CG touch my Abacus! When you're wrestling a gorilla, you don't stop when you're tired, you stop when the gorilla is.
As a boss please let me know that today is Friday I have a shipping target for Monday morning and I am busy finishing it, what would you like more : 1. I finish my target and deliver on time. 2. I find 10 issues in somebody's code and missed the target. My point is that if a responsibility is shared among multiple people with equal authority then nobody is actually responsible. My Startup!!!!
Profile@Elance - feedback available too -
As a boss please let me know that today is Friday I have a shipping target for Monday morning and I am busy finishing it, what would you like more : 1. I finish my target and deliver on time. 2. I find 10 issues in somebody's code and missed the target. My point is that if a responsibility is shared among multiple people with equal authority then nobody is actually responsible. My Startup!!!!
Profile@Elance - feedback available tooAmar Chaudhary wrote:
My point is that if a responsibility is shared among multiple people with equal authority then nobody is actually responsible.
That is a totally different point than that which you first made. My point still stands. Assuming I am the boss. If I tell you you will do code review, you will do code review or you won't work for me. If you feel that it will prevent you from hitting a deadline then you discuss that with your immediate line manager. Whatever their decision (make sure to get it in writing :) ), you adhere to it, or you don't work for me. If that decision causes the deadline to be missed then that's what it is.
Henry Minute Do not read medical books! You could die of a misprint. - Mark Twain Girl: (staring) "Why do you need an icy cucumber?" “I want to report a fraud. The government is lying to us all.” I wouldn't let CG touch my Abacus! When you're wrestling a gorilla, you don't stop when you're tired, you stop when the gorilla is.
-
Amar Chaudhary wrote:
My point is that if a responsibility is shared among multiple people with equal authority then nobody is actually responsible.
That is a totally different point than that which you first made. My point still stands. Assuming I am the boss. If I tell you you will do code review, you will do code review or you won't work for me. If you feel that it will prevent you from hitting a deadline then you discuss that with your immediate line manager. Whatever their decision (make sure to get it in writing :) ), you adhere to it, or you don't work for me. If that decision causes the deadline to be missed then that's what it is.
Henry Minute Do not read medical books! You could die of a misprint. - Mark Twain Girl: (staring) "Why do you need an icy cucumber?" “I want to report a fraud. The government is lying to us all.” I wouldn't let CG touch my Abacus! When you're wrestling a gorilla, you don't stop when you're tired, you stop when the gorilla is.
Henry Minute wrote:
If I tell you you will do code review
This is assigning responsibility to one single person, it is effective but declaring everybody will do the code review of peer programmer will lead to abuse of process. And that's what I said in initial post.
-
Henry Minute wrote:
If I tell you you will do code review
This is assigning responsibility to one single person, it is effective but declaring everybody will do the code review of peer programmer will lead to abuse of process. And that's what I said in initial post.
Amar Chaudhary wrote:
This is assigning responsibility to one single person,
No it is not. It is you deliberately assigning meaning to a phrase that was not originally intended. If I, as a manager decide that code reviews will be done, they will be done. Doesn't matter if by one or by all. If someone pays your wages and they assign a valid work related task to you, then you do it or leave. Either by your own volition, or otherwise. It matters not one jot if the task is the best method in your opinion. You said originally that you would decide to do code review or not. You were wrong in that statement, for reasons I have already stated.
Henry Minute Do not read medical books! You could die of a misprint. - Mark Twain Girl: (staring) "Why do you need an icy cucumber?" “I want to report a fraud. The government is lying to us all.” I wouldn't let CG touch my Abacus! When you're wrestling a gorilla, you don't stop when you're tired, you stop when the gorilla is.
-
Are you sure you mean parallel programming? Sounds more like pair programming, to me. Does someone not think that "pair" is a snazzy enough word?
I wanna be a eunuchs developer! Pass me a bread knife!
The parallel bit was my sad attempt at making a metaphor for design patterns as real world code review exercises. Parallel because you're utilizing all your cores (devs) vs singleton which is intentionally resource restricting (i.e. one person). If we were practicing pair programming we could eliminate the code review all together as that's the purpose of pair programming; doing the code review up front while you're coding. :)
- Arcond
-
52 is the new 42. :)
"Life can only be understood backwards, but it must be lived forward." Kierkegaard, Søren
913 x 613 = 4213 Which just show why everything seems to go wrong. The universe is based on an unlucky number.
-
913 x 613 = 4213 Which just show why everything seems to go wrong. The universe is based on an unlucky number.
Good point!
"Life can only be understood backwards, but it must be lived forward." Kierkegaard, Søren
-
The parallel bit was my sad attempt at making a metaphor for design patterns as real world code review exercises. Parallel because you're utilizing all your cores (devs) vs singleton which is intentionally resource restricting (i.e. one person). If we were practicing pair programming we could eliminate the code review all together as that's the purpose of pair programming; doing the code review up front while you're coding. :)
- Arcond
Don'tcha just hate it when you invent a perfectly logical term, only to discover that some dweeb has been using it for something else?
I wanna be a eunuchs developer! Pass me a bread knife!
-
I have a new team forming up and we want to decide on the best way to formalize code reviews. Is the Code Review the responsibility of one person or the whole team? Is each, or certain, team member(s) allowed to review others' code or is there only one person who does the reviews? If it's only one person, then who reviews his/her code? I've heard of both in practice and some of the teams where I'm at use both patterns. I'm just looking for insight as to what everyone thinks is the best way to go and why.
- Arcond
My opinion is that anyone can participate as a moderator, reader, or recorder. But if you want a meaningful review you need 1.) people familiar with the domain and 2.) people with the time to actually look at the material before the review starts. Otherwise just run it through some static analysis tool and don't bother with the review - it will just be a coding standards review anyway.
-
I have a new team forming up and we want to decide on the best way to formalize code reviews. Is the Code Review the responsibility of one person or the whole team? Is each, or certain, team member(s) allowed to review others' code or is there only one person who does the reviews? If it's only one person, then who reviews his/her code? I've heard of both in practice and some of the teams where I'm at use both patterns. I'm just looking for insight as to what everyone thinks is the best way to go and why.
- Arcond
The biggest challenge I've found with code reviews is people actually doing them long term. They'll run for a while, everybody will congratulate themselves on how useful they are, then at some people will stop doing it. And when people stop doing it, it's always for the same reason "We don't have time". This is partially true, but also hides the biggest reason, IMO: "We can't be bothered". Anybody that's ever done code reviews knows how much value they have, but the trouble is they can be time-consuming, tiresome, hardwork and boring. With that in mind, I'd recommend the following: 1. Don't have dedicated reviewers, because they will get bored quicker. Let everybody get involved; share the pain around. 2. Keep the reviews short and sweet. Split large reviews into multiple sessions. 3. Don't review in more detail than you really need. 4. Make sure everybody understand the benefits and are sold on the idea before you start. 5. Don't worry too much if you have a really busy period and fall behind on the reviews. Just let them go; draw a line in the sand and start again. Having said that, if you do have a senior dev that just loves doing code reviews and is happy to do them all, you might well be able to ignore all of the above. Just seems unlikely! Happy reviewing, Carl
-
I have a new team forming up and we want to decide on the best way to formalize code reviews. Is the Code Review the responsibility of one person or the whole team? Is each, or certain, team member(s) allowed to review others' code or is there only one person who does the reviews? If it's only one person, then who reviews his/her code? I've heard of both in practice and some of the teams where I'm at use both patterns. I'm just looking for insight as to what everyone thinks is the best way to go and why.
- Arcond
Start with a software tool for automated review. They can check for lines of code per procedure (e.g. 30 max), complexity (e.g. simple, moderate or high), a sufficient number of comments, parameter validation and error handling. Each team member can use it to review their own code before passing it to another team member for review. Don't track bugs found in self-review. After a developer says they are done with some code and have checked it in, another developer should review it for compliance with pre-defined standards. Bugs found in these reviews should be tracked in your bug tracking system. You should have more senior developers reviewing less experienced developer’s code and a senior developer review other senior developer’s code. Inexperienced developers (new hires out of college) or poor developers don't do code reviews. They should review bug tracking logs for code reviews to help understand code reviews and standards. After the code review passes, the code goes to test. QA could be a developer on your team. Always track bugs found here in your bug tracking system. The test process doesn't look at code. They just validate functionality. This process can also be automated with software tools. In a software project, all of these activities are going on throughout the process.
-
Transparent aluminum!
-
I have a new team forming up and we want to decide on the best way to formalize code reviews. Is the Code Review the responsibility of one person or the whole team? Is each, or certain, team member(s) allowed to review others' code or is there only one person who does the reviews? If it's only one person, then who reviews his/her code? I've heard of both in practice and some of the teams where I'm at use both patterns. I'm just looking for insight as to what everyone thinks is the best way to go and why.
- Arcond
We currently are trying to revive / implement Code Reviews. I’ve included a few links we’ve used as references. We had tried in the past to do formal reviews, in the beginning there was support but as schedules became tighter…it got left by the curb. We are now trying with informal reviews, and collecting metrics to support adding time to the project schedules for inspections. Realistically, there is not enough time to review everything. So guidelines help to consider potential risks like: The software … • Involves multiple 3rd Party development partners. • Involves new technologies or technologies unfamiliar to the development team. • Introduces a new or significantly revised software architecture. • Involves a complex architecture with many software assemblies and/or parts. • Includes parallel development of software assemblies and/or parts that share an interface (e.g. mutual communication channel). • Supports or will be reused by more than one software part, software assembly or software project. • Supports a safety critical software part, software assembly or software product. • Supports a feature or function that is critical to system operation (e.g. could result in a shutdown) • Supports a feature or function that is critical to the customer. • Is new or has been significantly revised (more than x %). • Will be developed by a contractor or 3rd party. • Will be developed by an inexperienced software developer. • Will be developed by a software developer who is new to the program. • Relies on multiple interactions between subsystems • Contains time critical features or functions We also use static code analysis to catch *simpler* common mistakes, code reviews focus on logic / requirements / functionality errors. http://www.ganssle.com/articles/inspectionspart1.htm http://www.processimpact.com/pr\_goodies.shtml http://www.crosstalkonline.org/search-all-issues/
Toto1107
-
I have a new team forming up and we want to decide on the best way to formalize code reviews. Is the Code Review the responsibility of one person or the whole team? Is each, or certain, team member(s) allowed to review others' code or is there only one person who does the reviews? If it's only one person, then who reviews his/her code? I've heard of both in practice and some of the teams where I'm at use both patterns. I'm just looking for insight as to what everyone thinks is the best way to go and why.
- Arcond
First define the goals of the review. Some or all of the following are possible. - Code bugs - Code quality (is it maintainable.) - Correct implementation (meets business requirements) - Security - Knowledge sharing. This can include product, business and/or technical. Once goals are defined then identify which participants for a review who would be necessary to meet the goals. For example interns are not necessarily participants unless the last item is in place. That said, one cannot review ones own code. Regardless of expertise another pair of eyes even without the necessary experience is going to provide some benefit. In my experience it also helps to emphasize that reviews are to be de-personalized. Enforcing this is a management role. For example if one person doesn't like where brackets are placed then, excluding code dev guidelines that specify it, then the manager must tell that individual that that personal preference is not be be brought up again. And participants must be encouraged, strongly, that issues in the code must be discovered. If someone is reviewing hundreds or thousands of lines of code and can't find a single thing wrong with it then they probably are not actually reviewing the code. Again this is something management must enforce, ideally, by using number of issues found in reviews (not created) as part of the employee review process.
