Code Review - thoughts
-
I think the biggest problem with good code reviews are finding reviewers that are familiar enough with the problem or subject matter to give good feedback. Since we are one deep in all positions where I work and everyone is working at 120% capacity that isn't easy. Without that code reviews degenerate into trivial complaints about variable naming, code factoring, and the like. For me having the time to do good unit testing is more valuable than code reviews.
tom1443 wrote:
I think the biggest problem with good code reviews are finding reviewers that are familiar enough with the problem or subject matter to give good feedback
Very good point.
tom1443 wrote:
good unit testing
Is always helpful and mandatory for us.
-
I think code / peer review is one of the most important aspects for any programmer who isn't working solo. My belief is based on the following reasons; A) Learning, a novice can be mentored by a developer with years of experience. An experienced developer could benefit from a novice just out of college, who may have been taught a new trick or two. B) Everyone makes mistakes, you are arrogant if you believe you cannot benefit from someone reviewing your code. Developers get into "patterns" of thought and much like a writer of a novel they could miss the obvious. C) Auditting & Security, it's important to the client that as a company you can show your practices are designed for quality, peer review is part of that. I understand that everyone has there own way of doing things but in my opinion thats a different discussion.
Simon Lee Shugar (Software Developer) www.simonshugar.co.uk "If something goes by a false name, would it mean that thing is fake? False by nature?" By Gilbert Durandil
Simon Lee Shugar wrote:
you are arrogant if you believe you cannot benefit from someone reviewing your code.
I agree with this statement very much. :thumbsup: It is sad, but there are many arrogant people in our field, and on this site. :sigh:
-
I am a big proponent for code reviews, prior to deployment. This is great if you work in a shop that has the people/resources to perform this and the time. Questions 1. Who here does not believe/practice code review? If so, please explain. 2. Who here works solo and has no immediate resources to code review? If so, would you use a third party review system (community)? Every great author has an editor, or should at least.
No arguments here. Agree that code reviews are important.
-
I am a big proponent for code reviews, prior to deployment. This is great if you work in a shop that has the people/resources to perform this and the time. Questions 1. Who here does not believe/practice code review? If so, please explain. 2. Who here works solo and has no immediate resources to code review? If so, would you use a third party review system (community)? Every great author has an editor, or should at least.
- When I make changes that affect a lot of behavior of the application, or I've had to discuss the changes with others at length, I have someone do a code review. The rest of the time I trust that I know what I'm doing. 2) Never, it would be a waste of my time. Nobody who doesn't understand the domain and structure of the rest of the program is going to be able to make any meaningful observations.
We can program with only 1's, but if all you've got are zeros, you've got nothing.
-
- When I make changes that affect a lot of behavior of the application, or I've had to discuss the changes with others at length, I have someone do a code review. The rest of the time I trust that I know what I'm doing. 2) Never, it would be a waste of my time. Nobody who doesn't understand the domain and structure of the rest of the program is going to be able to make any meaningful observations.
We can program with only 1's, but if all you've got are zeros, you've got nothing.
patbob wrote:
I trust that I know what I'm doing.
I hear you but that is a very, very dangerous stance. Just my opinion on that. ;)
-
Slacker007 wrote:
Who here does not believe/practice code review? If so, please explain.
I'm not a fan of code reviews. Usually: 1. What Jeremy said. And I'll be blunt - there's very few people that I think are qualified to review my code. Sorry to sound arrogant, because I'm not actually, but it's simply been my experience that code reviews tend to digress into "what's an anonymous method?", or "what is closure?" or "gee, I didn't know that was in the .NET framework." Granted, I HAVE been myself on that side of the fence, but a lot of people never seem to find the gate to the greener pasture. 2. When I write code, I'm in two states: the part that is writing it, and the part that is critiquing it. So, I'm my own reviewer, and you'll find in my open source projects a lot of TODO comments as a result, where I make a note for my future self to clean something up. Again, very few people do this, but I think one should be one's own reviewer.
Slacker007 wrote:
Who here works solo and has no immediate resources to code review?
Moi.
Slacker007 wrote:
If so, would you use a third party review system (community)?
No. Again, what Jeremy said regarding proprietary stuff. That said, I have several open source projects and of course a ton of articles, and any of those are open for critique. If the community wants to give me some feedback, that would be great, I'm all for an open dialog. Marc
Imperative to Functional Programming Succinctly Contributors Wanted for Higher Order Programming Project!
Marc Clifton wrote:
end to digress into "what's an anonymous method?", or "what is closure?" or "gee, I didn't know that was in the .NET framework."
Except of course that is in fact an avenue to teach other people about new concepts. On larger teams it also helps people be aware of changes in other parts of the system(s) which might be relevant to them but which they might not have otherwise become aware of.
-
I think code reviews would be great if they were done by someone who is at my level or above. And that's the problem. I won't say I'm the best coder you meet, but I can be pretty fanatic and look things up and do things 'different' and 'smart'. Not sure if I'm doing it right, but I wouldn't want to explain to a code reviewer why I use Generics in my Generics (and how that works), what the difference is between IQueryable and IEnumerable (and what actually happens in the back) and how we can construct Expressions manually. What I would like from a code reviewer is if he could tell me how to do things cleaner, better, faster, shorter and/or easier and still get the same result. My experience with most coders though (and that probably goes for most professions) is that they learn their trade (mostly at the job from 9 to 5) and rarely step out of their comfort zone after that (unless they have to). And so far at work I've been the best in what I do...
public class SanderRossel : Lazy<Person>
{
public void DoWork()
{
throw new NotSupportedException();
}
}Sander Rossel wrote:
but I wouldn't want to explain to a code reviewer why I use Generics in my Generics (and how that works), what the difference is between IQueryable and IEnumerable (and what actually happens in the back) and how we can construct Expressions manually.
Who do you expect to you maintain your code - you? For all time? If not and your code is 'above' the standard of your group then is it your expectation that your company is at fault for not hiring programmers who are not more syntactically advanced?
Sander Rossel wrote:
And so far at work I've been the best in what I do..
I have yet to work at a company where anybody said "our programmers are average" much less one that said "our programmers are below average". And very few programmers with any experience at all that were willing to admit that the code they wrote was less than excellent. Makes me wonder where all of those that must be below the curve are.
-
Marc Clifton wrote:
code reviews tend to digress into "what's an anonymous method?", or "what is closure?" or "gee, I didn't know that was in the .NET framework." Granted,
But, but ... but that is what is GOOD about code reviews! It's NOT just all aboutmaking sure your code is wonderful, it's about sharing the love - in both directions! Your reviewer may see code and say "I didn't know you could do it like that" or "Oh! I wouldn't have done it like that - why not do it like this" - and that then instigates a dialogue wherein you both ensure that this is the preferred way of doing it - either of you may be 'right' - even a junior programmer sometimes has a bright idea that you hadn't thought of or just didn't know about. Plus, if the other dev is more junior, this is their apprenticeship - your opportunity to help them grow by sharing your experiences. Sure, if they don't learn, and next time they look at your code they ask the same question, slap 'em, but generally sharing the ways we develop is a good and healthy experience. Where it does fall down is when two people who "aren't actually arrogant" look at the code and simply disagree on how it should be done. that's where a development manager comes in to adjudicate according to standards or even personal preference.
PooperPig - Coming Soon
_Maxxx_ wrote:
it's about sharing the love - in both directions!
Meh. That's a different meeting. Code reviews that degenerate into teaching sessions, especially when it then becomes clear that nobody in the room is qualified to review the code, and ESPECIALLY when someone makes some derogatory remark about how LINQ is unreadable and they'll stick to for loops and if statements in their code...but I digress...those are no longer code review meetings, IMO. :) And I've been to too many of those.
_Maxxx_ wrote:
Plus, if the other dev is more junior, this is their apprenticeship - your opportunity to help them grow by sharing your experiences.
More succinctly, code review and mentoring are two different processes. I have no problem if a junior member wants to sit in on a senior code review, but he/she should be quiet, take notes, and come to me for separate (or group, if that's the case, been there, done that) and I'm more than happy to help. Marc
Imperative to Functional Programming Succinctly Contributors Wanted for Higher Order Programming Project!
-
Marc Clifton wrote:
end to digress into "what's an anonymous method?", or "what is closure?" or "gee, I didn't know that was in the .NET framework."
Except of course that is in fact an avenue to teach other people about new concepts. On larger teams it also helps people be aware of changes in other parts of the system(s) which might be relevant to them but which they might not have otherwise become aware of.
jschell wrote:
Except of course that is in fact an avenue to teach other people about new concepts.
As I responded to _Maxxx_, there's a difference between a code review meeting and a mentoring meeting. I agree, certainly there's some learning that always goes on in a code review meeting, but it shouldn't be teaching a technology or a programming concept -- that falls into mentoring. Marc
Imperative to Functional Programming Succinctly Contributors Wanted for Higher Order Programming Project!
-
jschell wrote:
Except of course that is in fact an avenue to teach other people about new concepts.
As I responded to _Maxxx_, there's a difference between a code review meeting and a mentoring meeting. I agree, certainly there's some learning that always goes on in a code review meeting, but it shouldn't be teaching a technology or a programming concept -- that falls into mentoring. Marc
Imperative to Functional Programming Succinctly Contributors Wanted for Higher Order Programming Project!
I disagree. This assumes that you are right, your way is best, every time. I've come across exactly that attitude many times - and it's not a 'negative' arrogance, but it can be very misguided. As an example. A dev who is streets ahead of my level (and I'm a demi-god) had change a bunch of Linq code from var found = collectionClass.Where(i => i.Field == searchValue).FirstOrDefault(); to var found = collectionClass.FirstOrDefault(i => i.Field == searchValue); I asked why and was told it is more efficient. This is simply not true. (IN fact a coded loop is the more efficient, or using parallel, but that;s not the point!) That dev 'knew' he was right, to the point of changing code that worked, to be more efficient, when he as in fact making it less efficient. A code review not only stopped that, but allowed us to educate the team. (it also allowed me to point out that writing benchmarking programs and running them from the IDE in debug is a waste of time!)
PooperPig - Coming Soon
-
_Maxxx_ wrote:
it's about sharing the love - in both directions!
Meh. That's a different meeting. Code reviews that degenerate into teaching sessions, especially when it then becomes clear that nobody in the room is qualified to review the code, and ESPECIALLY when someone makes some derogatory remark about how LINQ is unreadable and they'll stick to for loops and if statements in their code...but I digress...those are no longer code review meetings, IMO. :) And I've been to too many of those.
_Maxxx_ wrote:
Plus, if the other dev is more junior, this is their apprenticeship - your opportunity to help them grow by sharing your experiences.
More succinctly, code review and mentoring are two different processes. I have no problem if a junior member wants to sit in on a senior code review, but he/she should be quiet, take notes, and come to me for separate (or group, if that's the case, been there, done that) and I'm more than happy to help. Marc
Imperative to Functional Programming Succinctly Contributors Wanted for Higher Order Programming Project!
Marc Clifton wrote:
it then becomes clear that nobody in the room is qualified to review the code,
So what do you think is the definition, in your environment, of 'good' code? If you get the situation where people are "not qualified" to review your code (why? because you are a god?) then you work in circumstances where teaching them better methods and/or ensuring that all code is able to be maintained by all developers is imperative! If you write some code that is just bloody brilliant, efficient, so superb nobody could write it better, but at your place of work there are developers who would struggle with it, then you are doing it wrong You need to either get these people up to speed, or start writing code that is easily maintainable by the people you work with and are likely to work with in the future. No question about it!
Marc Clifton wrote:
but he/she should be quiet, take notes, and come to me for separate
Oh, you arrogant fuck!
PooperPig - Coming Soon
-
I disagree. This assumes that you are right, your way is best, every time. I've come across exactly that attitude many times - and it's not a 'negative' arrogance, but it can be very misguided. As an example. A dev who is streets ahead of my level (and I'm a demi-god) had change a bunch of Linq code from var found = collectionClass.Where(i => i.Field == searchValue).FirstOrDefault(); to var found = collectionClass.FirstOrDefault(i => i.Field == searchValue); I asked why and was told it is more efficient. This is simply not true. (IN fact a coded loop is the more efficient, or using parallel, but that;s not the point!) That dev 'knew' he was right, to the point of changing code that worked, to be more efficient, when he as in fact making it less efficient. A code review not only stopped that, but allowed us to educate the team. (it also allowed me to point out that writing benchmarking programs and running them from the IDE in debug is a waste of time!)
PooperPig - Coming Soon
_Maxxx_ wrote:
A code review not only stopped that, but allowed us to educate the team.
I think that's a great example of a code review. Mentoring, in my definition, would be "here's how enumerables, extension methods, and lambda expressions work."
_Maxxx_ wrote:
it also allowed me to point out that writing benchmarking programs and running them from the IDE in debug is a waste of time!
:doh: Marc
Imperative to Functional Programming Succinctly Contributors Wanted for Higher Order Programming Project!
-
Marc Clifton wrote:
it then becomes clear that nobody in the room is qualified to review the code,
So what do you think is the definition, in your environment, of 'good' code? If you get the situation where people are "not qualified" to review your code (why? because you are a god?) then you work in circumstances where teaching them better methods and/or ensuring that all code is able to be maintained by all developers is imperative! If you write some code that is just bloody brilliant, efficient, so superb nobody could write it better, but at your place of work there are developers who would struggle with it, then you are doing it wrong You need to either get these people up to speed, or start writing code that is easily maintainable by the people you work with and are likely to work with in the future. No question about it!
Marc Clifton wrote:
but he/she should be quiet, take notes, and come to me for separate
Oh, you arrogant fuck!
PooperPig - Coming Soon
_Maxxx_ wrote:
then you work in circumstances where teaching them better methods and/or ensuring that all code is able to be maintained by all developers is imperative!
Absolutely. And I've done that.
_Maxxx_ wrote:
You need to either get these people up to speed, or start writing code that is easily maintainable by the people you work with and are likely to work with in the future.
Hmmm...code to the lowest denominator? I understand your point, and I think it has some merit, but I also think it is in everyone's interests to learn rather than stay stagnant in one's ignorance. Sadly, I've encountered a lot of shops where the managers will question why my team is using C# instead of VB: everyone else uses VB, and VB programmers are cheaper is the usual argument. :sigh: Would you say that I and my team should therefore code in VB?
_Maxxx_ wrote:
Oh, you arrogant f***!
Absolutely. I've been around the block enough to have experienced "their way" vs. "my way", and I have enough concrete examples of how "their way" led to disaster. In one case, "their way" lead to the client threatening to sue the company, and the only way the company got out of that pickle was to bring my team in as the tiger team and institute "my way." Ironically, most of their architecture was actually solid enough, it was the database architecture that was a disaster. Though, I actually wouldn't call it arrogance but rather hard earned experience. Marc
Imperative to Functional Programming Succinctly Contributors Wanted for Higher Order Programming Project!
-
jschell wrote:
Except of course that is in fact an avenue to teach other people about new concepts.
As I responded to _Maxxx_, there's a difference between a code review meeting and a mentoring meeting. I agree, certainly there's some learning that always goes on in a code review meeting, but it shouldn't be teaching a technology or a programming concept -- that falls into mentoring. Marc
Imperative to Functional Programming Succinctly Contributors Wanted for Higher Order Programming Project!
Marc Clifton wrote:
there's a difference between a code review meeting and a mentoring meeting
Not sure I have ever heard of the second. Certainly not in any company and not that I can recall in print. I have mentored people several times including officially. I have participated in tutorials as well. The first is suited to many things but the second doesn't work for some of the given examples mainly because is supposes first that tutorial meetings are occurring, that they occur often and that they present everything that is new. In comparison of course, code reviews should happen quite often. Finally of course new language concepts don't fit the concept of mentoring much either. For example a new DBA might be mentored by an existing DBA but there isn't much chance of either DBA learning that the sales department asked for a new entity and that UI people have hacked this in some persisted store but 'really soon now' it is going to become a full feature of the system.
