10 Commandments
-
IDEs than can't handle tabs with a selectable width should be eradicated. The one big problem with tabs is HTML insists on them representing 8 spaces. See PRE tags in a regular HTML page (I mean, beware CodeProject pages, as they tend to subtly change what you type!) :)
Luc Pattyn [Forum Guidelines] [Why QA sucks] [My Articles]
Prolific encyclopedia fixture proof-reader browser patron addict?
We all depend on the beast below.
Luc Pattyn wrote:
The one big problem with tabs is HTML insists on them representing 8 spaces.
I do so little with HTML that I wasn't aware of that. I'll try to remember against the day I take the plunge into Web development. I'll probably forget though. :( What were we talking about?
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.”
-
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? :~I'd say use a named constant anytime you can come up with a good functional name for it. Example: in C you might enumerate the days in a week, maybe they range from 0 to 6, maybe from 1 to 7. I don't want to know, just define FIRST_DAY_OF_WEEK and LAST_DAY_OF_WEEK, then do
for ( day = FIRST_DAY_OF_WEEK; day <= LAST_DAY_OF_WEEK ; day++ )
and it is obvious what the loop will do; with numbers you would have to worry about consistency throughout the app. And yes, there are several situations that don't warrant a symbol, as in your example. Zero is a rather special value. :)
Luc Pattyn [Forum Guidelines] [Why QA sucks] [My Articles]
Prolific encyclopedia fixture proof-reader browser patron addict?
We all depend on the beast below.
-
I'd say use a named constant anytime you can come up with a good functional name for it. Example: in C you might enumerate the days in a week, maybe they range from 0 to 6, maybe from 1 to 7. I don't want to know, just define FIRST_DAY_OF_WEEK and LAST_DAY_OF_WEEK, then do
for ( day = FIRST_DAY_OF_WEEK; day <= LAST_DAY_OF_WEEK ; day++ )
and it is obvious what the loop will do; with numbers you would have to worry about consistency throughout the app. And yes, there are several situations that don't warrant a symbol, as in your example. Zero is a rather special value. :)
Luc Pattyn [Forum Guidelines] [Why QA sucks] [My Articles]
Prolific encyclopedia fixture proof-reader browser patron addict?
We all depend on the beast below.
I actually do use contstants for very similar things to your example. Anywhere the number's meaning is not evident to another programmer.
-
I actually do use contstants for very similar things to your example. Anywhere the number's meaning is not evident to another programmer.
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.
-
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 one I've seen is this one[^] by Herb Sutter and Andrei Alexandrescu (two big names in C++) - forget naming conventions, talks more about how to design and code in a maintainable fashion.
Java, Basic, who cares - it's all a bunch of tree-hugging hippy cr*p CodeProject MVP for 2010 - who'd'a thunk it!
-
Because tabs are ambiguous. When viewed with tabs set to, say, 5 spaces, the comments might be misaligned. The IDE can handle spaces like tabs (more or less), so probably best to use those.
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.
-
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:
-
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 age old question... does length matter... Say I start working on a large solution (hundreds of files). Since the solution uses tabs, I have no idea what the intended spacing was. I assume 8 spaces, so I set tabs to use up 8 spaces. I start commenting at the end of code lines using tabs, and they line up fine for me. Say I work on 30 files like this. Then I come across a file where the comments don't line up. I find out it's because the previous person who worked on this was using 4 spaces for their tab length. I now have to go back and change my comments or all the other comments. And the reason to use tabs in comments like that is because it's faster than holding the space key. That problem could have been solved by using spaces in place of tabs. I personally don't care though. I pretty much never comment at the end of a line of code (I prefer to comment above code). I can see advantages to using tabs too. For example, if you are reading some heavily indented code, you could temporarily set the tab size to 1 space, which would make it easier to read. Both techniques have benefits, though the benefits of each are rarely applicable (at least, for me).
-
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).
If the "rule" is to "always declare a constant", then there should also be rules like: "Don't make constants public" ... or ... "Always rebuild all assemblies that reference public constants in other assemblies" Constants will get compiled "inline"; there are no references. If you change a public constant in an assembly (eg. a DLL), and don't recompile the applications that use, say, just constants from that assembly, those applications will continue to use the "old" values of those constants (that were compiled into their code). (Use a readonly field instead)
-
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 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).
If it's in C# then the standard MS guidelines are pretty good as far as coding standards go but in terms of a review we have a slightly higher level checklist as well which includes: 1) Have all aspects of the design been met 2) Is all of the code relevant to the design 3) Does the code meet the coding standards 4) Does the code clearly express its intention 5) Has the code been written without duplication 6) Is the code written using the fewest number of classes and methods It's really all about checking for duplicate and superfluous code as well as making sure that the code is clear and concise and well written, it also encourages self-documenting code. Another trick we try to use is to make the compiler do as much of the work for us as possible, so tell it to raise warnings as errors etc... also use coding standards such as placing literal values on the left of an equality operator, e.g.
if (0 == myValue) { }That way if an assignment operator is used instead of an equality one then the compiler will warn you about this as you can't assign a value to a literal. Used the other way around its a bug because an assignment will return a result of true and so the code will compile and run quite happily. -
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.