10 Commandments
-
Pascal Ganaye wrote:
This sort of thing: Pascal casing used for type and method names Camel casing used for local variable names and method arguments
This sort of things is best checked automatically by something like FxCop. Code reviews are about sharing knowledge and catching logic errors.
100% percent agree. The other advantage of code reviews is that they force you to code with the knowledge that you're going to have to justify what you write to someone else.
-
Then all is fine. Oh, another advantage of symbols is they help you search for things. If FIRST_DAY_OF_WEEK was not used but its value were 1, searching for 1 would hit a lot of things you're not looking for, whereas searching FIRST_DAY_OF_WEEK would only hit relevant instances. :)
Luc Pattyn [Forum Guidelines] [Why QA sucks] [My Articles]
Prolific encyclopedia fixture proof-reader browser patron addict?
We all depend on the beast below.
Snide comment from an OO zealot: So what's FIRST_DAY_OF_WEEK * FIRST_DAY_OF_WEEK then? </snide>
-
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;First rule: there is no cure for stupidity. Second rule: code review is meant to catch this type of stuff and figure out who the management problems are, and who needs training. If you are doing some level of review on a regular basis, you'll figure out who needs 'attention'. Rather than set an arbitrary set of lines of code for a method, I normally go by a rough cyclomatic complexity calc.. just score a point for every if condition in the method.. if it gets above 6 or 7 then better breakdown may be in order. Note that I've SEEN methods 1000s of lines long that exceed my rought cyclomatic calc complexity of 100. Some folks just can't be trusted with an editor/compiler. ;) Difficulty in maintenance is usually related to these things: Too much complexity in if conditions.. very difficult to understand. Too many methods for doing something simple.. you have to jump around and remember too much just to do something simple. Variables that are used for more than one purpose. Methods that have more than one goal. Incorrect usage of threads (typically lots of threads for little purpose). Lack of usage of locking mechanisms between threads Variable names that lack meaning. Lack of encapsulation of data in OO systems (I've seen this SO much). This one is a serious bugaboo.. since anything related to changing a datum in such a program means a review of the entire (or close to entire) program. Lots of copied code. Pointer errors. Non initialized variables. (I could go on.. this is just what at the top of my priority list for reviewing my code and others..) Keep in mind the goal of maintenance: minimum changes to localized code. If you are coding in a way that makes things not local or make changes required to be widespread, something is wrong in what you are doing.
-
Why even have a main body inside the for loop? Why not:
int i = 0; while (i < 100 && ReturnTrue(new MethodInvoker(delegate { DoSomething(i++); }))) ;
:rolleyes:
-
Does that matter? Tabs should be used for indentation, spaces should be used for lining things up. The size of indentation does not matter at all. That's one of the advantages of tabs - The user can set their preferred tab size.
The problem is when different people edit the same files using varying setups - some using tabs, some using spaces, plus varying tab interpretations (4, 3, 8, etc ...). The end result is an indentation nightmare that looks as if a monkey has been dancing on your keyboard.
-
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).
-
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
I would always put spaces around binary operator, it's more readable. And you can use the prefix ++ operator in a for loop exactly as the postfix operator is used, but the prefix is more efficent cause it doesn't have to return the previous value, although i would assume the compiler probably optimizes this well.
Pete
-
From the Microsoft one: "Do not use a prefix for member variables (_, m_, s_, etc.). If you want to distinguish between local and member variables you should use “this.” in C# and “Me.” in VB.NET" What? I thought the .NET guidelines said to use an underscore? :wtf:
Although I agree with them (I have always disliked prefixes on member variables), I do understand where you are coming from. M$ has always been consistently inconsistent. :)
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.”
-
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.
I agree, go the whole StyleCop standard holus bolus. That way you avoid holy wars, its an arbitrary standard that Im sure you'll disagree with some parts of,b ut you will disagree with parts of any standard. After using it for a month I'm totally used to it.
-
Why even have a main body inside the for loop? Why not:
int i = 0; while (i < 100 && ReturnTrue(new MethodInvoker(delegate { DoSomething(i++); }))) ;
:rolleyes:
I used to code that way, I got tired of being the only one to be able to understand my code. In designing electronic circuits, I learned that if you want the technician to correctly build what you intend the circuits need to be drawn in their classic configurations ie: a voltage divider with the resistors one over the other, op-amps input on the left output on the right. It is the way they learned to use and understand them. The most complex code is build of simple structures. HoleyMoley
-
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
-
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).
I prefer to ignore all formatting arguments. I've given up on trying to enforce tab rules - every editor seems to handle them differently. Most languages have pretty-printers, and/or they're not horribly difficult to create. Here's what I have come up with for our standards (we're primarily C but some C#). Much of it is very subjective. The programmer should be allowed significant flexibility, with the most experienced, even-handed developer having the final say. Commenting: File header blocks Header blocks for functions and substantial data structures Adequate comment frequency and quantity Legible and helpful comments (perfect spelling/grammar not required) Accurate All commented-out code is worth retaining and explained Coding Style: Standard and consistent Braces around single-statement blocks (to eliminate macro-inserted logic bugs) Magic (hard-coded) numbers minimized Constant declarations used where appropriate Meaningful variable names (i is sometimes appropriate) Global and local symbols distinguished Architecture: Efficiency and maintainability tradeoff Encapsulation and cohesion Global and shared data minimized Entry and exit points minimized Non-standard or non-portable techniques avoided, or documented when used KISS Reasonable function length Reasonable nesting depth Exception conditions pre-empted or handled No redundant code No dead code All build warnings eliminated or addressed
-
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).
I had to do this at my last gig, only the shop had a very ingrown and long-established culture..."the design is in the code", "if the code is right, no one has to read it again", etc.. The approach I took which was at least moderately successful, was to first take the MS standards as the starting point -- they are pretty good and more importantly they set the tone for what is important and what is not. Stay away from the silly stuff like cunting comments or the number of spaces inside of comments and concentrate on what makes reliable, reusable, and readable. The second and more important aspect was to get the affected people involved early in a series of ongoing review/feedback opportunities so everyone felt like they had a chance to contribute. These don't have to be meetings - just post the in-progress standards somewhere and let people know they are available and you are taking comments. Those that are truly interested will participate, those that aren't ... they missed their chance. Also use 'guest authors' for specialty areas like .MSIs and SPROCs -- they know what is really important in their areas of specialty. We wound up with a set of standards and 'advisory' items which everyone pretty much adhered to, and that required a minimal amount of maintenance/updating on my part as MS went through its regular convulsions (er, new releases). Finally, make it clear that you have management support and that adherence to the final standards will be part of automated code reviews and individual performance reviews (you did get management buy-in at the beginning, right?). And of course, don't forget your body armor :suss:
“The trouble ain't that there is too many fools, but that the lightning ain't distributed right.” --Mark Twain
-
The problem is when different people edit the same files using varying setups - some using tabs, some using spaces, plus varying tab interpretations (4, 3, 8, etc ...). The end result is an indentation nightmare that looks as if a monkey has been dancing on your keyboard.
This looks like someone making his jobs easier, by making everyone else job harder. As I see it, as long as all the people on a project agree to a tab size there is no problem. And using spaces instead of tabs carry the problem that you will need to press the keyboard 4 to 8 times(depending on tab size) to indent and un-indent your code when with a tab you press only once. When in doubt, just put a comment line telling whoever wants to maintain your code your tab size.
-
You can just configure the IDE to replace tabs with spaces whenever you type them... Then again, it can do most of the indenting on its own too!
Yep, kind of my point. who needs to really worry about spaces when the IDE will fill them in for you. I'd rather be concerned about thought process while coding than to worry about how many spaces I've put in.
"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
-
This looks like someone making his jobs easier, by making everyone else job harder. As I see it, as long as all the people on a project agree to a tab size there is no problem. And using spaces instead of tabs carry the problem that you will need to press the keyboard 4 to 8 times(depending on tab size) to indent and un-indent your code when with a tab you press only once. When in doubt, just put a comment line telling whoever wants to maintain your code your tab size.
rxantos wrote:
And using spaces instead of tabs carry the problem that you will need to press the keyboard 4 to 8 times
IDE's allow you to press TAB to insert spaces and SHIFT+TAB to unindent. It's just a matter of setting the correct option. You do not need to press the space key.
-
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).
I think the thing to emphasise in this kind of document is what the goal is, rather than trying to lay down detailed constraints on things like code layout. Ultimately they are down to personal preference, and trying to dictate them invariably leads to arguments. Instead try and set some overall principles. For example, here's how I began the coding standards for one of my previous employers: There are three guiding principles: • Code for documentation. The code is part of the design documentation. • Code for debugging. The code should be written so as to make debugging easy. • Code for modification. The code should be written so as to be readily modified. I also added this: Remember that a source file is for human consumption - it is part of the program's design documentation. At the very least it will be read by your colleagues and your boss, so: • The normal rules of spelling and grammar apply. • Keep it tidy. • Don’t try to be funny or rude. So when code reviewing the key question is “is this code sufficiently clear and understandable that another team member could modify it?” It is not “how many spaces have been used for indentation?” Can I also point you at ‘The Pragmatic Programmer’ by Andrew Hunt and David Thomas. It contains lots of useful advice on how software development should be done. They also have a web site here: http://www.pragprog.com/.
-
rxantos wrote:
And using spaces instead of tabs carry the problem that you will need to press the keyboard 4 to 8 times
IDE's allow you to press TAB to insert spaces and SHIFT+TAB to unindent. It's just a matter of setting the correct option. You do not need to press the space key.
-
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 best advice that I can offer is to decide on what factors are important to your (current) group of developers and what is not. Them try to standardize the things that you feel are important in the way that most (hopefully all) are willing to work with. There are a lot of good ideas for points to consider in the other discussion here and the links that have been provided. However, it is your group that has to live with the end result. You need to come up with something that everyone can buy into or it may never work. I have participated and even driven this process several times in the past. It can be very helpful if nobody takes it too seriously and everyone takes it seriously enough. Good luck!
-
There was once a time when I didn't know that either.