|
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.
|
|
|
|
|
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.
|
|
|
|
|
I've been a developer for several decades. I get value from my code being reviewed by others.
My ideas on reviewing of other people's code should be obvious.
====
On a related note, one of my observations is that every contributor will search until they find something to criticize. One of my review techniques (for my code submissions) is to make sure that there is at least one blatant mistake that everybody can find, which can be "fixed" with a trivial action on my part; else if your submission is truly close to perfect, you will get several new requirements to add wheel spinning features that have little (or negative) value.
Windows 8 is the resurrected version of Microsoft Bob. The only thing missing is the Fisher-Price logo.
- Harvey
|
|
|
|
|
A few of my thoughts:
1) Code reviews are good.
2) If you think your code doesn't need to be reviewed - Then it needs to be reviewed
3) As you said, code reviews are for everyone, Say you write perfect code, what better way for someone else to learn something they didnt know of
4) More and more, static analysis tools can check for coding standards, unfortanately most of the comments in reviews relate to naming conventions and comments :/
5) I like to focus on correctness of code and design for integration with the system and future maintainability.
To know and not do, is not yet to know
http://www.codeofthedamned.com
|
|
|
|
|
What a silly idea. I'm a man, I know what I am doing and there is no reason for anyone else to look at my perfect code. I've never made a mistake and I never will.
|
|
|
|
|
You're married... when was the last time you were right about anything???
|
|
|
|
|
That is not related to marriage. I know this because I'm in fact not married but living in sin and still incorrect about everything.
|
|
|
|
|
|
Given the amount I've heard lately about her preferences in diamond settings I suspect she would prefer tomato
|
|
|
|
|
You're getting the hint then? Wait, are you currently in custody up here at Indooroopilly??? (If this makes no sense, google Indooroopilly diamond theft.)
|
|
|
|
|
_Damian S_ wrote: You're getting the hint then? Wait, are you currently in custody up here at Indooroopilly??? (If this makes no sense, google Indooroopilly diamond theft.)
Josh would need several layers of sun tan to be the diamond ring pooper man.
Michael Martin
Australia
"I controlled my laughter and simple said "No,I am very busy,so I can't write any code for you". The moment they heard this all the smiling face turned into a sad looking face and one of them farted. So I had to leave the place as soon as possible."
- Mr.Prakash One Fine Saturday. 24/04/2004
|
|
|
|
|
True that... maybe it was his brilliant disguise?
|
|
|
|
|
_Damian S_ wrote: (If this makes no sense, google Indooroopilly diamond theft.)
Sorry, I was in the bathroom with constable smellmyturd
|
|
|
|
|
Get back into the kitchen with the other girlies
MVVM # - I did it My Way
___________________________________________
Man, you're a god. - walterhevedeich 26/05/2011
.\\axxx
(That's an 'M')
|
|
|
|
|
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')
|
|
|
|
|
I've had excellent experience with Code Reviews.
We have found a number of bugs that would have gone unnoticed if not for code reviews, and I'm a strong advocate of having code reviews.
"Real men drive manual transmission" - Rajesh.
|
|
|
|
|
It's a great way of learning more about what it is that you're making, which can help you to do your own bits easier.
I wanna be a eunuchs developer! Pass me a bread knife!
|
|
|
|
|
Suggest you agree the standards and process in advance - by discussion not by dictat.
|
|
|
|
|
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.
|
|
|
|
|
_Maxxx_ wrote: and down if you think they are the devil's work!
not fair! Downvoting is disabled.
I guess it kinda depends on how, when, etc the code reviews are done.
|
|
|
|
|
_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
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')
|
|
|
|
|
_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
If you can't read my code, try converting it here[^]
|
|
|
|
|
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
|
|
|
|