|
Cornelius Henning wrote: but I must admit that I have learnt from a few constructive criticisms of the few articles I wrote here on CP!
I'm actually surprised how few critiques I see in general, other than when it's blatantly obvious. My most humbling experience was this[^] 
Marc
|
|
|
|
|
Marc Clifton wrote: but a lot of people never seem to find the gate to the greener pasture.
Interestingly enough man, I find this concept applies to so many aspects of life that extends way beyond the reach of just programming. It's like the more you know the more you accept the less you know, and are ok with that.
Marc Clifton wrote: 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.
I do the same thing! Ha. My problem is though is getting back to those TODOs and making the changes.Marc Clifton wrote: If the community wants to give me some feedback, that would be great, I'm all for an open dialog.
That's probably how I got my distaste for reviews. One of things that kept me from writing more articles on CP for instance is having to deal with the "expert kiddos" that try to sound smart critiquing your code. It's annoying arguing with them. Waste of my time actually. But, I'd want the mature peer reviews by other respectable programmers. Tough to have your cake and eat it too.
Jeremy Falcon
|
|
|
|
|
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
|
|
|
|
|
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
|
|
|
|
|
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.
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.
|
|
|
|
|
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
|
|
|
|
|
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 f***!
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.  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
|
|
|
|
|
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.
|
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
