|
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
|
|
|
|
|
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!
Marc
|
|
|
|
|
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.
|
|
|
|
|
Slacker007 wrote: I am a big proponent for code reviews,
Yay!
Slacker007 wrote: prior to deployment
How does that work when you havent got any to review? 
Code review is THE single most important quality tool you have. A second set of eyes, and engineer, going over the code, looking at logic, working out why it is doing what it is dong, and asking the dev all he needs to ask, can pull out a LOT of bugs.
In fact, you can take all the SW design, all the process flow diagramns, all the dozygen crap, and throw it away. A code review is far more important.
|
|
|
|
|
Munchies_Matt wrote: Code review is THE single most important quality tool you have.
I think code reviews fall into three categories:
1. Does it meet the functional requirements?
2. Does it meet the architectural requirements?
3. Does it meet usability requirements?
Item #1 can be determined with unit and integration tests, which I find is far superior and repeatable than code reviews.
Item #2, well, there's the problem, because it assumes an architecture and someone that holds the architecture and reviews code to see if it complies. One of the reasons I'm a software architect is because I find that role to be greatly lacking in all the non-solo projects I've ever worked on, and is actually why I can see that, even in my early 20's, I started to fill that role.
Item #3, well, only the user can usually tell you that. 
Marc
|
|
|
|
|
Getting a sanity check on the code I've written. Why wouldn't I want that?
Isn't that the supposed advantage with open source?
|
|
|
|
|
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...
My blog[ ^]
public class SanderRossel : Lazy<Person>
{
public void DoWork()
{
throw new NotSupportedException();
}
}
|
|
|
|
|
Sander Rossel wrote: I think code reviews would be great if they were done by someone who is at my level or above.
Is there any other person? I have never had nor seen a code review done by someone with a "weaker" skill set. I code review peers that are at my level or below; usually at my level.
|
|
|
|
|
Maybe I should rephrase that:
I think code reviews would be great if they were done by someone who is at my level or above rather than by someone who thinks he's better or who is assumed to be better than me (or whoever wrote the code).
And that happened a few times in my case. The reviewer, technical director even, who came questioning me for using interfaces and asking why I created an interface AND a base class and then wanted some sources stating that you should use both... 
And how are you going to measure who is better anyway? Years of experience? Job title? Both are pretty weak as the person I just talked about had eight years more experience and was technical director while I had one year experience and was a junior
My blog[ ^]
public class SanderRossel : Lazy<Person>
{
public void DoWork()
{
throw new NotSupportedException();
}
}
|
|
|
|
|
I just had a code review done on my code, right this minute, by our principal Engineer. Ship it he says.
If he said "Steve, why are you using Interfaces here and there?" I would explain myself. If he didn't agree with my reasons, I would change it. Why? because that is the way successful software companies/shops work. We are a team.
Good luck to you and your career. 
|
|
|
|
|
If my boss didn't agree with me using good programming practices, such as relying on abstractions, then that's not a company/team I want to work for (unless, of course, there's a very good reason not to do it). The problem in my above example wasn't that I used an interface, but that I used an interface and implemented it in a base class that could be inherited. The interface was there for people who did not need the base class or who had already inherited from other classes. This was actually a little tool that would be used by different programmers and in different solutions, so you need to be SOLID (another set of principles I had to defend) and make use of design patterns (I still hear the guy screaming "IN MY EIGHT YEARS OF EXPERIENCE I'VE NEVER HEARD OF DESIGN PATTERNS BEFORE! SHOW ME THAT MICROSOFT USES THEM AND I'LL RECONSIDER YOUR CODE!"). The next day he came to apologize and he had looked up some design patterns that Microsoft uses in .NET. In the end I got his job (sort of) and he and I weren't put on the same team anymore (he was still my boss, co-owner of the company).
I actually owe the guy quite a bit since he taught me programming (the first few months). I made up by writing pretty awesome software (that was something we all agreed on) that the company has successfully used for years.
In hindsight I could've dealt with him a bit different (I called his code a card house), but I was young and arrogant (and I still am a bit) and let's just say he and I didn't go well together.
We thanked each other last month when I left the company (on good terms) to learn at another company. I guess all's well that ends well
Slacker007 wrote: Good luck to you and your career.
Thanks, it's going pretty swell actually!
You too, of course. Have fun with the code reviews
My blog[ ^]
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.
|
|
|
|
|
I have yet to see regular working review...
However something that I practice which works just as well, is regular "pair programming" not as in "today we are going to work together" but more like, "hey can you help me or share your thoughts on that problem"
It helps at solving problem, sharing code style and knowledge! 
|
|
|
|
|
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. 
It is sad, but there are many arrogant people in our field, and on this site. 
|
|
|
|
|
1) As has been mentioned a few times already finding other devs who actually engage during reviews is hard. A better option I've found (after spending multiple months trying to get the team to participate in reviews rather than just say "yup that's fine") is pair programming. Either a full on formal approach where you have all devs working in pairs or a less formal approach where you identify features that will be complicated to implement and have two people pair up to work on it. I've found that the devs are more engaged and it's easier to spot the bad stuff as it's being written.
2) Pretty much only works if you're going the full open-source route. Otherwise, as has been previously stated, you'll probably run afoul of NDAs and IP rights.
|
|
|
|
|
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.
|
|
|
|
|
Both 1 and 2, but because I was the sole developer for a few years.
Now, there's another young person working with me. For the first bit, while he was getting going understanding the app/data/environment/etc, we'd discuss approaches and I'd at least give a quick glance at checkins and discuss alternatives.
He's proven himself to be quite competent though. Now it's when one of us knows we're doing something a bit off, we'll talk with the other..."what do you think about this approach".
It seems to be working well. For such a small company, I don't think we have (or choose not to have) the resources for a more formal review structure. This makes me sad, as I know getting more eyes on the project would only make it stronger.
|
|
|
|
|
I agree, that with small companies, things have to be tweaked in such a way that it works.
|
|
|
|
|
Love 'em, but I do a lot of one man coding myself.
Honestly, I have my editor(s) setup to do a lot of the formatting automatically,
and to reformat my code.
I can't share my code because of NDAs as mentioned elsewhere. But that does NOT mean you
cannot review your own code! As a note, we did all of our code reviews on Fridays. Usually
after lunch, through the end of the day. It was designed to discourage working over the weekend,
and to help us unwind and not be stressed out. I felt it was the perfect way to end the week.
But before coded reviews are started, coding standards must exist. If you have those, then I would
take some time out at the end of the week, and review your own code. Eventually you will find it is second nature. Also, once we determined something to be dangerous (or bug inducing), we updated our coding standards to prevent it. Like declaring a stack based buffer of a fixed size to read in from a stream of unknown length (buffer overflow, anyone?).
I think if you review your own code, and look it over, and would be proud to show it to anyone who asked... You are ahead of 70% of the coders out there. Also, when you find yourself referring to your old code, you will appreciate it and reuse it more...
Kirk Out!
|
|
|
|
|
Code reviews are great for showing new team members "how we do it here".
I usually work alone, but will occasionally ask a colleague, "what do you think of this method/function/whatever?"
On the other hand I'm mostly working with SSIS now, and that isn't code.
|
|
|
|
|
No arguments here. Agree that code reviews are important.
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
