10 Commandments
-
I have been asked to work on our Code Review Procedure. This sort of thing: Pascal casing used for type and method names Camel casing used for local variable names and method arguments ... A single file should only contribute types to a single namespace. Avoid having multiple namespaces in the same file. Avoid files with more than 500 lines (excluding machine-generated code). Avoid methods with more than 25 lines. ... Never hard-code a numeric value, always declare a constant instead. ... I have the feeling it is missing what really make maintenance go bad. Things like re use existing classes, don't duplicate code, separate logic, data access, and display. Can anyone point me to some documents with good recommendations of this type. The idea is to minimize defect and certainly not to alienate the programmers (including me).
Pascal Ganaye wrote:
Never hard-code a numeric value, always declare a constant instead
Be careful with this one; sometimes you get morons who do this:
const int IntZero = 0;
const double DblZero = 0.0;
const int IntOne = 1;
const double DblOne = 1.0;
const int IntTwo = 2;
...Software Zen:
delete this; -
I have been asked to work on our Code Review Procedure. This sort of thing: Pascal casing used for type and method names Camel casing used for local variable names and method arguments ... A single file should only contribute types to a single namespace. Avoid having multiple namespaces in the same file. Avoid files with more than 500 lines (excluding machine-generated code). Avoid methods with more than 25 lines. ... Never hard-code a numeric value, always declare a constant instead. ... I have the feeling it is missing what really make maintenance go bad. Things like re use existing classes, don't duplicate code, separate logic, data access, and display. Can anyone point me to some documents with good recommendations of this type. The idea is to minimize defect and certainly not to alienate the programmers (including me).
The things you have mentioned in the first part of you post are already checked by FxCop [^] and StyleCop[^] and can be automated. They may or may not directly impact the number of defects. FxCop to some extent finds certain things which might be problematic. Using StyleCop, on the other hand, you can enforce all the styling. This will prepare you for the next step which is Manual Code Review. Manual Code Review should be to catch subtle things: not protecting shared variables, potential memory leak, better way to do a certain thing (use an API instead of writing code to implement the functionality of API). Code Review can catch lot of subtle bugs and that is why it is so effective. Also, if there are many programmers in your team it is essential that you use automated tool to analyze the style and code before they sit and do a code review. This is because programmers usually get nit-picky of style and miss the more important things. If you have common auto enforceable guidelines, it helps you get to the next step.
-
Pascal Ganaye wrote:
Never hard-code a numeric value, always declare a constant instead
Be careful with this one; sometimes you get morons who do this:
const int IntZero = 0;
const double DblZero = 0.0;
const int IntOne = 1;
const double DblOne = 1.0;
const int IntTwo = 2;
...Software Zen:
delete this;I personally would prefer Don't hard-code a non trivial numeric value that appears, declare a constant instead. This is particularly true when the value appears more than once.
-
:wtf: I agree with most of the MS one... how'd that happen? :confused:
-
I have been asked to work on our Code Review Procedure. This sort of thing: Pascal casing used for type and method names Camel casing used for local variable names and method arguments ... A single file should only contribute types to a single namespace. Avoid having multiple namespaces in the same file. Avoid files with more than 500 lines (excluding machine-generated code). Avoid methods with more than 25 lines. ... Never hard-code a numeric value, always declare a constant instead. ... I have the feeling it is missing what really make maintenance go bad. Things like re use existing classes, don't duplicate code, separate logic, data access, and display. Can anyone point me to some documents with good recommendations of this type. The idea is to minimize defect and certainly not to alienate the programmers (including me).
Pascal Ganaye wrote:
Avoid methods with more than 25 lines.
That seems awfully low. Maybe for this newfangled stuff where you make a color gradient animation in one line of code, and all people goo "ooh" and "aah" and forget to ask what that software is for, but for real work? (admittedly, that's from the guy who has to maintain a 4KLOC switch statement)
Agh! Reality! My Archnemesis![^]
| FoldWithUs! | sighist | µLaunch - program launcher for server core and hyper-v server. -
Pascal Ganaye wrote:
Never hard-code a numeric value, always declare a constant instead
Be careful with this one; sometimes you get morons who do this:
const int IntZero = 0;
const double DblZero = 0.0;
const int IntOne = 1;
const double DblOne = 1.0;
const int IntTwo = 2;
...Software Zen:
delete this;And after the same morons start doing maintenance, you'll see stuff like:
const int IntTwo = 3;
-
And after the same morons start doing maintenance, you'll see stuff like:
const int IntTwo = 3;
If I found someone doing that, I would be hard pressed not to nuke their cube from orbit; it's the only way to be sure.
Software Zen:
delete this; -
I have been asked to work on our Code Review Procedure. This sort of thing: Pascal casing used for type and method names Camel casing used for local variable names and method arguments ... A single file should only contribute types to a single namespace. Avoid having multiple namespaces in the same file. Avoid files with more than 500 lines (excluding machine-generated code). Avoid methods with more than 25 lines. ... Never hard-code a numeric value, always declare a constant instead. ... I have the feeling it is missing what really make maintenance go bad. Things like re use existing classes, don't duplicate code, separate logic, data access, and display. Can anyone point me to some documents with good recommendations of this type. The idea is to minimize defect and certainly not to alienate the programmers (including me).
-
Pascal Ganaye wrote:
Avoid methods with more than 25 lines.
That seems awfully low. Maybe for this newfangled stuff where you make a color gradient animation in one line of code, and all people goo "ooh" and "aah" and forget to ask what that software is for, but for real work? (admittedly, that's from the guy who has to maintain a 4KLOC switch statement)
Agh! Reality! My Archnemesis![^]
| FoldWithUs! | sighist | µLaunch - program launcher for server core and hyper-v server.peterchen wrote:
That seems awfully low
Not in the Java/C#/Linq world. For instance, in my code base of around 100,000 Lines, the maximum number of lines in a manually written method is 28. I think 25-30 is a reasonable maximum. Some Java frameworks have as much as 15 as the maximum.
-
:wtf: I agree with most of the MS one... how'd that happen? :confused:
Take two Aspirin, lie down in a dark room and if not better by morning, call the doctor.
Henry Minute Do not read medical books! You could die of a misprint. - Mark Twain Girl: (staring) "Why do you need an icy cucumber?" “I want to report a fraud. The government is lying to us all.”
-
I personally would prefer Don't hard-code a non trivial numeric value that appears, declare a constant instead. This is particularly true when the value appears more than once.
Pascal Ganaye wrote:
Don't hard-code a non trivial numeric value that appears, declare a constant instead. This is particularly true when the value appears more than once.
Any advice for how to best organize such declarations so they make sense? To be sure, automated tools make the layout less important than it would have been in the days when printouts were used a lot more, but I often find that I have constants which really don't "fit" in any of my "constants" areas. Also, btw, in VB I sometimes end up using constants cb1, cb2, etc. in places where I need a small byte value, since VB will squawk at "bytevar = bytevar or 1" but will accept "bytevar = bytevar or ByteConstantWhichEqualsOne" without complaint. What would have been nicer would be if there were a suffix to denote that a particular value should be regarded as a byte, but no such thing exists.
-
peterchen wrote:
That seems awfully low
Not in the Java/C#/Linq world. For instance, in my code base of around 100,000 Lines, the maximum number of lines in a manually written method is 28. I think 25-30 is a reasonable maximum. Some Java frameworks have as much as 15 as the maximum.
Rama Krishna Vavilala wrote:
I think 25-30 is a reasonable maximum.
I hate large methods as much as the next guy, but in several cases, I have methods running to almost 200 lines. On the code base I'm working on, it seems counter-intuitive to break it up to several methods.
If the post was helpful, please vote, eh! Current activities: Book: Devils by Fyodor Dostoyevsky Project: Hospital Automation, final stage Learning: Image analysis, LINQ Now and forever, defiant to the end. What is Multiple Sclerosis[^]?
-
I have been asked to work on our Code Review Procedure. This sort of thing: Pascal casing used for type and method names Camel casing used for local variable names and method arguments ... A single file should only contribute types to a single namespace. Avoid having multiple namespaces in the same file. Avoid files with more than 500 lines (excluding machine-generated code). Avoid methods with more than 25 lines. ... Never hard-code a numeric value, always declare a constant instead. ... I have the feeling it is missing what really make maintenance go bad. Things like re use existing classes, don't duplicate code, separate logic, data access, and display. Can anyone point me to some documents with good recommendations of this type. The idea is to minimize defect and certainly not to alienate the programmers (including me).
Pascal Ganaye wrote:
Never hard-code a numeric value, always declare a constant instead.
Yikes! What about
if errors.Count() > 0? Surely I don't need a ZERO_ONLY_ZERO constant? :~ -
Look into tools like FxCop, StyleCop and ReSharper. These should take care of formatting and redundant code and things like that. Once you are done with doing all this look into the logic if it adheres to the requirement.
+100 ReSharper. I just love that software
If the post was helpful, please vote, eh! Current activities: Book: Devils by Fyodor Dostoyevsky Project: Hospital Automation, final stage Learning: Image analysis, LINQ Now and forever, defiant to the end. What is Multiple Sclerosis[^]?
-
Rama Krishna Vavilala wrote:
I think 25-30 is a reasonable maximum.
I hate large methods as much as the next guy, but in several cases, I have methods running to almost 200 lines. On the code base I'm working on, it seems counter-intuitive to break it up to several methods.
If the post was helpful, please vote, eh! Current activities: Book: Devils by Fyodor Dostoyevsky Project: Hospital Automation, final stage Learning: Image analysis, LINQ Now and forever, defiant to the end. What is Multiple Sclerosis[^]?
Mustafa Ismail Mustafa wrote:
counter-intuitive to break it up to several methods.
That is where I normally take opinion of someone else. Someone, figures out a clever way, usually. The best way to break a method is in chunks of logic. The bad way is to do the following:
void Method()
{
Portion1();
Portion2();
Portion3();
} -
:wtf: I agree with most of the MS one... how'd that happen? :confused:
I agree with most of them, but one that really bugs me is:
for (int i=0; i<100; i++) { DoSomething(i); }I prefer:for (int i=0; i<100; i++) DoSomething(i);or:for (int i=0; i<100; i++)
{
DoSomething(i);
}With the last one, who knows what future statements may need to be added... Another one that bugs me is stating that 4 spaces should be used instead of Tab, sorry, Tab tab tab ;P
"The clue train passed his station without stopping." - John Simmons / outlaw programmer "Real programmers just throw a bunch of 1s and 0s at the computer to see what sticks" - Pete O'Hanlon "Not only do you continue to babble nonsense, you can't even correctly remember the nonsense you babbled just minutes ago." - Rob Graham
-
peterchen wrote:
That seems awfully low
Not in the Java/C#/Linq world. For instance, in my code base of around 100,000 Lines, the maximum number of lines in a manually written method is 28. I think 25-30 is a reasonable maximum. Some Java frameworks have as much as 15 as the maximum.
Hmm... I've checked a few of recent functions. Observations: It's C++, which has a notable overhead mostly for error handling. However, in most cases I don't see any benefit of breaking them up. One of them, quite simple by this project's standards, is 62 lines, there are 14 opening/closing braces, 6 full-line comments helping understand the code flow, and 11 empty lines to group "logic blocks" (each of the full-line comment is preceded by one). Breaking up the functionality (finding a best match) into three logical pieces was possible (Check A, Check B, Generate Diagnostics), but that would expose a lot of context. With the additional boilerplate, documentation and definition of the functions' contracts, I'd say I'd end up with worse code. Of course, being a C++ programmer, I could write that code in 25 lines, but you really don't want me to :rolleyes:
Agh! Reality! My Archnemesis![^]
| FoldWithUs! | sighist | µLaunch - program launcher for server core and hyper-v server. -
Mustafa Ismail Mustafa wrote:
counter-intuitive to break it up to several methods.
That is where I normally take opinion of someone else. Someone, figures out a clever way, usually. The best way to break a method is in chunks of logic. The bad way is to do the following:
void Method()
{
Portion1();
Portion2();
Portion3();
}Rama Krishna Vavilala wrote:
That is where I normally take opinion of someone else. Someone, figures out a clever way, usually. The best way to break a method is in chunks of logic.
Well yeah, and normally that's the way I'd prefer it to go, but sometimes, you've got your back to the wall...
Rama Krishna Vavilala wrote:
void Method() { Portion1(); Portion2(); Portion3(); }
X| :thumbsdown: I'll admit in my younger days I did that a few times until a friendly senior explained to me some of the facts of programming life.
If the post was helpful, please vote, eh! Current activities: Book: Devils by Fyodor Dostoyevsky Project: Hospital Automation, final stage Learning: Image analysis, LINQ Now and forever, defiant to the end. What is Multiple Sclerosis[^]?
-
I have been asked to work on our Code Review Procedure. This sort of thing: Pascal casing used for type and method names Camel casing used for local variable names and method arguments ... A single file should only contribute types to a single namespace. Avoid having multiple namespaces in the same file. Avoid files with more than 500 lines (excluding machine-generated code). Avoid methods with more than 25 lines. ... Never hard-code a numeric value, always declare a constant instead. ... I have the feeling it is missing what really make maintenance go bad. Things like re use existing classes, don't duplicate code, separate logic, data access, and display. Can anyone point me to some documents with good recommendations of this type. The idea is to minimize defect and certainly not to alienate the programmers (including me).
-
Hmm... I've checked a few of recent functions. Observations: It's C++, which has a notable overhead mostly for error handling. However, in most cases I don't see any benefit of breaking them up. One of them, quite simple by this project's standards, is 62 lines, there are 14 opening/closing braces, 6 full-line comments helping understand the code flow, and 11 empty lines to group "logic blocks" (each of the full-line comment is preceded by one). Breaking up the functionality (finding a best match) into three logical pieces was possible (Check A, Check B, Generate Diagnostics), but that would expose a lot of context. With the additional boilerplate, documentation and definition of the functions' contracts, I'd say I'd end up with worse code. Of course, being a C++ programmer, I could write that code in 25 lines, but you really don't want me to :rolleyes:
Agh! Reality! My Archnemesis![^]
| FoldWithUs! | sighist | µLaunch - program launcher for server core and hyper-v server.When I maintain someone's code, I will prefer to see CheckA(); CheckB(); GeneralDiagniostics(); Seeing that I may get an idea of what the method is doing even though I may be new to the team. However, it may become worse if you are passing lot of parameters around. The thing I like about such rules of enforcing maximum lines is programmers are made to think and when they think good solutions may emerge. The eventual aim is to have fewer methods and fewer classes in other words "less code". Less code less bugs.