Code Review - thoughts
-
Marc Clifton wrote:
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
What about as a tool to share experience; have other people look at your code as a learning and share expertise in your organization; if you are not able to explain your code to other people, then, maybe your code is not good enough to be maintained; and if, and it is possible, the person who's doing your code review is not at the same technical level as you are, then, tell the person who assigned the code review. I'm not the best or brightest, and I like looking at other people's code, even if I need to review code that I'm not 100% at ease with.
I'd rather be phishing!
Maximilien wrote:
What about as a tool to share experience; have other people look at your code as a learning and share expertise in your organization;
I'd be interested in that and have even contemplated the idea of putting a website together that would facilitate that exchange, both privately for proprietary code and publicly for open source reviews. Tying in with, say, GitHub, to then track changes to reviewed code, well, that could be a useful, maybe even revenue generating, tool.
Maximilien wrote:
f you are not able to explain your code to other people, then, maybe your code is not good enough to be maintained;
Amen to that. One of the reasons I love writing documentation is that, in explaining what I'm doing, I often discover bugs and better ways to do it. Now, granted, some people simply struggle with communicating, but I would have to say, I have yet to see the person who, failing to communicate in English (or whatever their native tongue is) actually produce decent code.
Maximilien wrote:
the person who's doing your code review is not at the same technical level as you are, then, tell the person who assigned the code review.
This involves too much of a "management" structure that, being a consultant, I happily avoid.
Maximilien wrote:
I'm not the best or brightest, and I like looking at other people's code, even if I need to review code that I'm not 100% at ease with.
Same here. I have learned a lot from other people's code, here on CP and a lot on Stack Overflow and people's blogs. Marc
Imperative to Functional Programming Succinctly Contributors Wanted for Higher Order Programming Project!
-
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!
I work in a medium sized software shop, and NONE of our code makes it to the deployment pipeline, unless it has been code reviewed. Whether you have been coding for 5 minutes or 25 years, your code is getting reviewed. Most of the time, the review is just making sure you are not committing unsafe code, and not so much on coding style. Making sure that the Entity Framework models, services, and other things have not been put in a state that would break the build or cause problems down range. Edit: I do not agree with the coder being their own reviewer. I have been doing this for over 15 years, and as good as I am at my job, I still make mistakes. Mistakes that were caught by my peers. Just saying. :)
-
I work in a medium sized software shop, and NONE of our code makes it to the deployment pipeline, unless it has been code reviewed. Whether you have been coding for 5 minutes or 25 years, your code is getting reviewed. Most of the time, the review is just making sure you are not committing unsafe code, and not so much on coding style. Making sure that the Entity Framework models, services, and other things have not been put in a state that would break the build or cause problems down range. Edit: I do not agree with the coder being their own reviewer. I have been doing this for over 15 years, and as good as I am at my job, I still make mistakes. Mistakes that were caught by my peers. Just saying. :)
Slacker007 wrote:
I work in a medium sized software shop, and NONE of our code makes it to the deployment pipeline, unless it has been code reviewed.
I'm impressed. It's nice to see an organization actually invest in the process.
Slacker007 wrote:
Mistakes that were caught by my peers.
Out of curiosity, could those mistakes also have been caught by unit / integration testing, or would that have been prohibitively complicated to set up -- in other words, having a human being do the code review is much more cost effective? Marc
Imperative to Functional Programming Succinctly Contributors Wanted for Higher Order Programming Project!
-
- No code reviews. I used to, but the medication keeps the other personalities at bay... 2) I work solo, but I wouldn't want external code reviews - most of the stuff I do is proprietary or covered by NDA / corporate confidentiality agreements and I wouldn't let it into the public domain.
Bad command or file name. Bad, bad command! Sit! Stay! Staaaay...
OriginalGriff wrote:
- No code reviews. I used to, but the medication keeps the other personalities at bay...
You beat me to it. Only I was going to say that I had multiple personalities; 1) Coder guy 2) QA guy 3) Code review guy 4) Beast
New version: WinHeist Version 2.1.0 My goal in life is to have a psychiatric disorder named after me. I'm currently unsupervised, I know it freaks me out too but the possibilities are endless.
-
Slacker007 wrote:
I work in a medium sized software shop, and NONE of our code makes it to the deployment pipeline, unless it has been code reviewed.
I'm impressed. It's nice to see an organization actually invest in the process.
Slacker007 wrote:
Mistakes that were caught by my peers.
Out of curiosity, could those mistakes also have been caught by unit / integration testing, or would that have been prohibitively complicated to set up -- in other words, having a human being do the code review is much more cost effective? Marc
Imperative to Functional Programming Succinctly Contributors Wanted for Higher Order Programming Project!
Marc Clifton wrote:
Out of curiosity, could those mistakes also have been caught by unit / integration testing, or would that have been prohibitively complicated to set up -- in other words, having a human being do the code review is much more cost effective?
So our process is this: 1. code/test 2. Prior to committing the code to the trunk: run all automated unit tests. If those are successful, then submit for code review. When that is cleared for take off, the merge changes to trunk. 3. This kicks off an automated build, another set of tests are done, and then deploy to DEV server. 4. Testing is done by the business team/QA in both DEV and STG. 5. When that is signed off on, then we have a release readiness meeting to move the changes to PRD. 6. After it is in PRD, we do a smoke test...and hope for the best. :laugh: Funny, forgot to answer your question. Yes, unit tests, regression testing, integration testin catches issues but the code has to be deployed to an environment first in order to do integration testing, usually DEV. If we find problems, then everything has to be rolled back to the last successful build, and that can be a pain in the ass, if 20 other engineers have code in the pipeline as well. Most of the time, things go well, but they go well because we have a process.
-
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.
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?
Wrong is evil and must be defeated. - Jeff Ello
-
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.
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();
}
} -
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:
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.
-
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... :doh: 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 :)
public class SanderRossel : Lazy<Person>
{
public void DoWork()
{
throw new NotSupportedException();
}
} -
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... :doh: 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 :)
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. :thumbsup:
-
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. :thumbsup:
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. :thumbsup:
Thanks, it's going pretty swell actually! :thumbsup: You too, of course. Have fun with the code reviews :)
public class SanderRossel : Lazy<Person>
{
public void DoWork()
{
throw new NotSupportedException();
}
} -
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.
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! :)
All in one Menu-Ribbon Bar DirectX for WinRT/C# since 2013! Taking over the world since 1371!
-
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:
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
-
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.
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
-
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.
- 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 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.
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.
-
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.
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 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.
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!
-
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.
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. :sigh:
-
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.
