Code Reviews
-
Reviewing other people's code is fun to an extent exponentially related to the size of their ego and organizational title. yours, Bill
"What Turing gave us for the first time (and without Turing you just couldn't do any of this) is he gave us a way of thinking about and taking seriously and thinking in a disciplined way about phenomena that have, as I like to say, trillions of moving parts. Until the late 20th century, nobody knew how to take seriously a machine with a trillion moving parts. It's just mind-boggling." Daniel C. Dennett
Spot on :thumbsup:
“That which can be asserted without evidence, can be dismissed without evidence.”
― Christopher Hitchens
-
In general I have good experience with code reviews - not as much as a tool for discovering bug as a mechanism for sharing knowledge. However, as with most things in software engineering, you shouldn't go overboard with them. I've been in situations where code reviews were taking ridiculous amount of time and energy. In most cases, a code review should take one round of comments and responses, and the committer should be allowed to disregard the comments unless he/she is a junior developer.
I can't agree more. Sadly I have been mostly at companies which did it wrong and were code reviews took days. They actually didn't code reviews, they looked at how they should have built it and then tell you you did it wrong. If you didn't comply with the architecture, that wouldn't have been a problem ofcourse, but the code always complied. The there-are-multiple-ways-to-Rome idea missed its road from their building to Rome, because they really didn't get it and always complained they didn't have enough time to do their work, because they had to review everything... A good code review looks for any problems, any deviations from required standards and some tips/hints. You should clearly separate those groups of comments. Problems and deviations need to be fixed. Tips/hints should be considered helpful and not a kick in the butt.
-
I am quite keen on doing code reviews at my current place of employment - although I have not worked anywhere that does code reviews before. My belief is that code reviews will help ensure standards are kept, enhance the learning of all the developers, and help drive any changes in standards. The other developers think it is a waste of time except for new developers having their code reviewed (presumably until they can be trusted) Your thoughts/experiences/advice? As a blatant gamble for more points, vote this message up if you think Code reviews are a Good Thing, and down if you think they are the devil's work!
MVVM # - I did it My Way ___________________________________________ Man, you're a god. - walterhevedeich 26/05/2011 .\\axxx (That's an 'M')
Code reviews can be a very good idea, provided that the standards the code is to adhere to are agreed in advance, and everybody remembers that the purpose is to improve code quality, not score points by nit-picking every tiny detail to make the author look like an idiot and everyone else feel "big and strong". Used well, they are an definite plus: used badly they are a tool for workplace bullying and can destroy morale in a department.
-
I am quite keen on doing code reviews at my current place of employment - although I have not worked anywhere that does code reviews before. My belief is that code reviews will help ensure standards are kept, enhance the learning of all the developers, and help drive any changes in standards. The other developers think it is a waste of time except for new developers having their code reviewed (presumably until they can be trusted) Your thoughts/experiences/advice? As a blatant gamble for more points, vote this message up if you think Code reviews are a Good Thing, and down if you think they are the devil's work!
MVVM # - I did it My Way ___________________________________________ Man, you're a god. - walterhevedeich 26/05/2011 .\\axxx (That's an 'M')
_Maxxx_ wrote:
and down if you think they are the devil's work!
not fair! Downvoting is disabled. ;P I guess it kinda depends on how, when, etc the code reviews are done.
-
I am quite keen on doing code reviews at my current place of employment - although I have not worked anywhere that does code reviews before. My belief is that code reviews will help ensure standards are kept, enhance the learning of all the developers, and help drive any changes in standards. The other developers think it is a waste of time except for new developers having their code reviewed (presumably until they can be trusted) Your thoughts/experiences/advice? As a blatant gamble for more points, vote this message up if you think Code reviews are a Good Thing, and down if you think they are the devil's work!
MVVM # - I did it My Way ___________________________________________ Man, you're a god. - walterhevedeich 26/05/2011 .\\axxx (That's an 'M')
_Maxxx_ wrote:
My belief is that code reviews will help ensure standards are kept,
I love reviews, but not for the sake of a "standard". I could not care less about whether you think each instance member needs be prefixed with
this., whether private members are preceded with the term "private". If it does not add value (and only cost), then the best practice would state to not enforce the standards.Bastard Programmer from Hell :suss: If you can't read my code, try converting it here[^]
-
_Maxxx_ wrote:
My belief is that code reviews will help ensure standards are kept,
I love reviews, but not for the sake of a "standard". I could not care less about whether you think each instance member needs be prefixed with
this., whether private members are preceded with the term "private". If it does not add value (and only cost), then the best practice would state to not enforce the standards.Bastard Programmer from Hell :suss: If you can't read my code, try converting it here[^]
well there are standards, and there are standards. My view of any standard is that it should have a reason for existing. If there is a good reason for using this. over not using this. then it should be adopted as a standard. If there is no compelling reason then there shouldn't be a standard. The important thing to remember, though, is that one man's "but its obvious" is another man's "Wow! I didn't realise" - so having standards with explanations is a good idea - especially for those less experienced in that area of code. A good example, I think, is in Wpf - where many a standard says "thou shalt not write any code behind" well that is obviously bollocks (IMHO) and should be more like "thou shalt not put business logic in the code behind". And that is where a code review comes in handy, as then more than one pair of eyes can look at any code and decide if it is business logic or not.
MVVM # - I did it My Way ___________________________________________ Man, you're a god. - walterhevedeich 26/05/2011 .\\axxx (That's an 'M')
-
I am quite keen on doing code reviews at my current place of employment - although I have not worked anywhere that does code reviews before. My belief is that code reviews will help ensure standards are kept, enhance the learning of all the developers, and help drive any changes in standards. The other developers think it is a waste of time except for new developers having their code reviewed (presumably until they can be trusted) Your thoughts/experiences/advice? As a blatant gamble for more points, vote this message up if you think Code reviews are a Good Thing, and down if you think they are the devil's work!
MVVM # - I did it My Way ___________________________________________ Man, you're a god. - walterhevedeich 26/05/2011 .\\axxx (That's an 'M')
Do I think code reviews should be done and can be useful? Yes... However, before a code review can be done to
_Maxxx_ wrote:
ensure standards are kept
, then said standards must have been developed ahead of time and the reasoning for the standard should be described in the standard. "Because I said so" is not a reason. If you are going to write code that is going to be reviewed, be prepared to both document and explain in person any processing that others may not have seen. In the late '80s I was involved in rewriting two systems to move them to a new platform. Code review was in place for both rewrites. One of the 'standards' was: all file names must be 9 characters and the routine name in the file must match the file name. Since the first 3 characters described the system, that left 6 characters to define a meaningful file and routine name. This standard was quickly abandoned for routine names because they were much more cryptic than meaningful; this was for a system with over 500 routines in a single executable. And the reasoning for the standard? The old platform was limited in the length of a file name size, and the standards developers didn't check the system capabilities on the new platform. While reviewing code (FORTran), I encountered something I had never seen before (a multiple return) and asked the developer about it. His reply, "It's a multiple return". He either could not or would not describe it in any other terms. I finally replied, "And telling me a car is a car is only useful if I understand what a car is." At that point, he said, "I'll rewrite the code". Again, nothing wrong with the code, but the developer could not explain what is was doing. Just my thoughts... Tim
-
well there are standards, and there are standards. My view of any standard is that it should have a reason for existing. If there is a good reason for using this. over not using this. then it should be adopted as a standard. If there is no compelling reason then there shouldn't be a standard. The important thing to remember, though, is that one man's "but its obvious" is another man's "Wow! I didn't realise" - so having standards with explanations is a good idea - especially for those less experienced in that area of code. A good example, I think, is in Wpf - where many a standard says "thou shalt not write any code behind" well that is obviously bollocks (IMHO) and should be more like "thou shalt not put business logic in the code behind". And that is where a code review comes in handy, as then more than one pair of eyes can look at any code and decide if it is business logic or not.
MVVM # - I did it My Way ___________________________________________ Man, you're a god. - walterhevedeich 26/05/2011 .\\axxx (That's an 'M')
_Maxxx_ wrote:
well there are standards, and there are standards.
A standard means that you follow Microsofts' guidelines[^], as opposed to inventing your own "standard". Nine of ten times, the terms coding convention and code reviews means a subset of forementioned document, without any argumentation and a lot of fruitless discussions.
_Maxxx_ wrote:
My view of any standard is that it should have a reason for existing.
FxCop will give it to you as soon as you break one of those rules. That should give enough indication on whether or not the deviation is logical or not.
_Maxxx_ wrote:
If there is a good reason for using this. over not using this. then it should be adopted as a standard.
Stop talking about it, follow the standard, omit it. The same goes for declaring things "private"; learn what the default access modifier is for your language, and start following the standard. Omitting the obvious makes code a lot more readable. ..and that's why we wanted that standard in the first place.
_Maxxx_ wrote:
having standards with explanations is a good idea - especially for those less experienced in that area of code.
Making code readable does far more to help the less experienced. And any standard without argumentation should be dismissed without argumentation.
Bastard Programmer from Hell :suss: If you can't read my code, try converting it here[^]
-
I am quite keen on doing code reviews at my current place of employment - although I have not worked anywhere that does code reviews before. My belief is that code reviews will help ensure standards are kept, enhance the learning of all the developers, and help drive any changes in standards. The other developers think it is a waste of time except for new developers having their code reviewed (presumably until they can be trusted) Your thoughts/experiences/advice? As a blatant gamble for more points, vote this message up if you think Code reviews are a Good Thing, and down if you think they are the devil's work!
MVVM # - I did it My Way ___________________________________________ Man, you're a god. - walterhevedeich 26/05/2011 .\\axxx (That's an 'M')
_Maxxx_ wrote:
The other developers think it is a waste of time except for new developers having their code reviewed (presumably until they can be trusted)
They are wrong.
_Maxxx_ wrote:
My belief is that code reviews will help ensure standards are kept, enhance the learning of all the developers, and help drive any changes in standards.
Standards mean little in terms of actual quality unless reviews, not just code reviews, are taken seriously. Reviews of the entire process lead to many types of improvements where as style guidelines have a very small impact. They do increase learning. One is exposed different expressions immediately rather than just via maintenance. It also insures adherence to requirements in that reviews at least have the possibility that they might cause a failure of a requirement in another part of the system, before that system reaches testing (which would hopefully demonstrate it regardless.)
-
In general I have good experience with code reviews - not as much as a tool for discovering bug as a mechanism for sharing knowledge. However, as with most things in software engineering, you shouldn't go overboard with them. I've been in situations where code reviews were taking ridiculous amount of time and energy. In most cases, a code review should take one round of comments and responses, and the committer should be allowed to disregard the comments unless he/she is a junior developer.
Nemanja Trifunovic wrote:
and the committer should be allowed to disregard the comments
Review ticket items should be categorized as one of the following - security bug - bug - comment - well done Security bugs must be fixed before the code goes to production. And it must be reviewed for the fix. Bugs can be triaged but with the goal to fix. Comments are suggestions about alternate ways to do something. There need not be any action at all. Well done (or create your own term) is an indication that the reviewer really likes the way a piece of code works.
-
But seriously, either your workmates think like this or they are scared of being shown up by the new guys
A little of both, I think. Many closed minds.
MVVM # - I did it My Way ___________________________________________ Man, you're a god. - walterhevedeich 26/05/2011 .\\axxx (That's an 'M')
