BREAK as many C# Coding Standards / Good Practice as possible please
-
Hi,<?xml:namespace prefix = o ns = "urn:schemas-microsoft-com:office:office" /><o:p></o:p>
This seems the place to ask. Has anyone got a simple 15-30 line c# example of BREAKING as many c# coding standards/good practices as possible? I am trying to set a little challenge to 'find all the mistakes' competition for my team. <o:p></o:p>
If I find a good one elsewhere, I will publish it here :)<o:p></o:p>
Thanks<o:p></o:p>
Dave
-
I don't have such program ready, but here is a checklist (obviously to be completed):
Java-like use of upper and lower case
Java-like getters/setters
public fields
manualy looping through an IEnumerable (instead of foreach)
building strings in a loop using the "+" operator (instead of using a StringBuilder)
unnecessary imports
empty catch blocks
-
Thanks ammoQ,<?xml:namespace prefix = o ns = "urn:schemas-microsoft-com:office:office" /><o:p></o:p>
Great start to the list. I was just thinking it may just be a single method. Perhaps adding:<o:p></o:p>
DB connections without a 'using' passing out a datareader from the method.<o:p></o:p>
Magic number usage for some parameter or setting.<o:p></o:p>
Username and password in the connection string.<o:p></o:p>
Dave
-
@BeepBeep said:
DB connections without a 'using' passing out a datareader from the method.<o:p></o:p>
Now would be a good time to ask - on what basis do I determine if an object should be instantiated with a "using" keyword?
I never really understood why "using" exists in the first place. I mean, doesnt the GC take care of this kinda stuff automatically?@BeepBeep said:
Magic number usage for some parameter or setting.
What do you mean? And how do you solve this "problem"?
PS: In an ironic way, while most of you folks visit this website to mock bad programming practices... I always seem to learn good techniques via their corollary :)
-
What about these?
Iterating through an array and catching an exception to denote the end of the array has been reached!!
Calling abstract methods from the constructor in the base classUsing lazy initialisation to construct/return a singleton object to be used by multiple threads
-
Gizmo I apologise, you are absolutely correct. It is pointless listing mistakes without saying why I/We considering them mistakes.<?xml:namespace prefix = o ns = "urn:schemas-microsoft-com:office:office" /><o:p></o:p>
'DB connections without a 'using' passing out a datareader from the method.'<o:p></o:p>
When you use a 'Using' the runtime implicitly disposes of the resources scoped by the using. In the case of a connection (sqlConnection) this closes the connection to the database if it is open and disposes of the unmanaged connection.<o:p></o:p>
The 'passing out of a datareader' is interesting because the datareader is a forward-only reader and is connected to the database during its reading 'DataReader.Read()'. Passing it out of the current method can create weird errors due to the connection being open and the fact that there could only be 1 open reader on a connection at a time (this may have been fixed in ADO.net 2).<o:p></o:p>
Current 'good practice' (I will refrain from saying 'best practice') is to open the connection at the last possible moment and close it as soon as possible. Whilst this seems to be a waste, connection pooling can increase the efficiency of database calls significantly. (<FONT color=#0000ff>http://aspalliance.com/1099_Understanding_Connection_Pooling_in_NET</FONT>)
-
Pete, love the first one:
Iterating through an array and catching an exception to denote the end of the array has been reached!!
The other are a little to deep for a 30 line example and I never give anyone a hard time for threading bugs, though the current fashion for singletons everywhere is worrying.
-
Gizmo,<?xml:namespace prefix = o ns = "urn:schemas-microsoft-com:office:office" /><o:p></o:p>
Using a 'Using' statement scopes calls the object instance's Dispose when the using braces close. So if an object references unmanaged resources (connections, files, com components) AND the object releases then within its own dispose method then using the Using statement is a nice clean technique.
-
An overload of an operator that seriously goes against the usual meaning of the operator.
In the language we already have i = j << 2; and cout << "hello";
In the library we aready have x = a[3]; for an array and a = x["smith"] for a map.
How about this: where p is an instance of a class P representing a persion, with fields such as age, define p++ as incrementing the age field. That way you call this function only once a year. Then ++p can be defined to increment the age of the person's spouse.
-
Here's an example of a class:
class Person {
protected: Role role;
public:
Person () { role = new Role(); }
~Person() { delete role; }
}
The problem here occurs with code like this:
std::map<int, Person> people;
people[emplNum] = new Person(); // Already there's a memory leak! Even if people was empty.
The Rule of the Big Three: If you need a destructor, or a copy constructor, or a copy assignment operator, you need all three.
-
@ammoQ said:
building strings in a loop using the "+" operator (instead of using a StringBuilder)
I know it's really bad to do that, but I seriously don't think I've ever used StringBuilder. Ok, well, I have used it a couple times when I needed the performance, but I've just found it too annoying and generally could care less about performance since I'm usually just using the +'s in debug logging anyway. Is there really any other reason to use StringBuilder other than performance and because you are supposed to?
-
@Pete Guyatt said:
Iterating through an array and catching an exception to denote the end of the array has been reached!!
Might as well just extend it to say using exceptions for ANY kind of operational logic...
void SomeStupidFunc(int someStupidParam)
{
switch(someStupidParam)
{
case 0: throw new IsOneException();
case 1: throw new IsTwoException();
case 2: throw new IsThreeException();
default: throw new IsInvalidException();
}
}
void MainFunc()
{
int i = 5;
try
{
SomeStupidFunc(i)
} catch (IsOneException eX) {
i += 2;
NextFunc(i);
} catch (IsTwoException eX) {
i -= 4;
NextFunc(i);
} catch (IsThreeException eX) {
i *= 5;
NextFunc(i);
} catch (IsInvalidException eX) {
i = 5;
} catch (Exception eX) {
MessageBox("An error occured"); // ;-P LOL!
}
}
-
@newfweiler said:
An overload of an operator that seriously goes against the usual meaning of the operator.
In the language we already have i = j << 2; and cout << "hello";
In the library we aready have x = a[3]; for an array and a = x["smith"] for a map.
How about this: where p is an instance of a class P representing a persion, with fields such as age, define p++ as incrementing the age field. That way you call this function only once a year. Then ++p can be defined to increment the age of the person's spouse.
...
Hot.
-
How about using return values of functions instead of catching exceptions?
You could have a method like the following:
bool SomeMethod()
{
try
{
int a = 0;
int b = 1 / a;
}
catch (OverflowException)
{
return false;
}
return true;
}
And then use it like this:
if (SomeMethod())
{
// Success
}
else
{// Failure
}
-
@GoatCheez said:
@ammoQ said:
building strings in a loop using the "+" operator (instead of using a StringBuilder)
I know it's really bad to do that, but I seriously don't think I've ever used StringBuilder. Ok, well, I have used it a couple times when I needed the performance, but I've just found it too annoying and generally could care less about performance since I'm usually just using the +'s in debug logging anyway. Is there really any other reason to use StringBuilder other than performance and because you are supposed to?
It comes down to wether or not you'll feel the impact, really. If you're looping through hundreds or thousands of records and, say, [i]building a string[/i] out of them... then use StringBuilder. If it's a one-off thing like the title of the current page/window or an error message, then string concatenation is fine.
You have to know when to trade processor time for programmer time...
-
@GoatCheez said:
@Pete Guyatt said:
Iterating through an array and catching an exception to denote the end of the array has been reached!!
Might as well just extend it to say using exceptions for ANY kind of operational logic...
As sad as that is, I've seen it in production code. Using C++ exceptions to return success values... failures usually resulted in program crashes.
-
@GoatCheez said:
@ammoQ said:
building strings in a loop using the "+" operator (instead of using a StringBuilder)
I know it's really bad to do that, but I seriously don't think I've ever used StringBuilder. Ok, well, I have used it a couple times when I needed the performance, but I've just found it too annoying and generally could care less about performance since I'm usually just using the +'s in debug logging anyway. Is there really any other reason to use StringBuilder other than performance and because you are supposed to?
If your code looks like this:
logger.log("foobar: found "+n+" records");
using StringBuilder would IMO mean over-engineering.
On the other hand, code like this should better use a StringBuilder:
string buffer = "";
foreach(string x in foobar) {
buffer = buffer + x + ','
}
-
@Pete Guyatt said:
Using lazy initialisation to construct/return a singleton object to be used by multiple threads
That's not a problem if you have a legitimate reason to use lazy initialisation, for instance if the singleton represents the CD drive and the user only sometimes needs to insert the CD (rather than being required to insert the CD every time the program is run).
Initialize the singleton pointer to null. In the accessor, fetch the pointer. If the pointer is null, enter a critical section. Fetch the pointer again. If not null, another thread has constructed it; leave the critical section and return the pointer. Otherwise create the object, assign it to the pointer, leave the critical section and return the pointer. [For "critical section" read "mutex" or whatever synchronization method is appropriate; they have slightly different meanings in different OS's.]
Now the code that uses the drive just has to call the accessor. (Now you have to deal with operational problems: what if the user doesn't have a blank disc, or whatever is needed? Can you wait an indefinite period of time? Can you give the operator the option to cancel the operation without cancelling the program? Should the accessor throw an exception in this case? Should the accessor "remember" the drive is not available (so the operator can never burn a disc even after he comes back with blank discs) or should it "forget" (and ask the operator again and again and again and again)? No amount of theory can tell you the answer to those questions.)
Singletons are perfectly appropriate for modeling singletons, like network cards or CD drives. Sometimes you want an "n-gleton" when there are exactly 2 or 3 things, like 2 network cards or 2 CD drives. For most programs you want "the" network connection or "the" CD drive even if the computer has two or three. A network bridge uses exactly two network cards, so it has either two singletons, the first and second network cards, or one singleton which is a container which contains exactly two network cards.
Just remember there are a few things that started out as singletons but now come in pairs or worse. Central processing units for example.
-
@newfweiler said:
An overload of an operator that seriously goes against the usual meaning of the operator.
In the language we already have i = j << 2; and cout << "hello";
In the library we aready have x = a[3]; for an array and a = x["smith"] for a map.
How about this: where p is an instance of a class P representing a persion, with fields such as age, define p++ as incrementing the age field. That way you call this function only once a year. Then ++p can be defined to increment the age of the person's spouse.
The WTF, of course, is that the age is stored as the current value and updated each year rather than calculated from the date of birth ;-)
-
@newfweiler said:
In the library we aready have x = a[3]; for an array and a = x["smith"] for a map.
IMO this is not bad; from a more abstract point of view, the array is a special kind of map.
-
The point is not that x = a[3] and a = x["smith"] are bad practice, but that they are already overloaded, so there is no need to define them. (I think so.)
C# is actually quite restrictive of the types of bizarre constructions you can invent. I think in regular C++ there are many more ways to create atrocities. Perhaps the most cruel one arises from the fact that the comma is also an operator, and therefore we can have an OVERLOADED COMMA OPERATOR!! <insert mad laugh>
class Bar { public: Bar(int x=0) : val(x) {} int val; } class Foo { public: Foo(int x=0) : val(x) {} int operator,(Bar &x) { val += x.val; return val; } int val; } ostream &operator<<(ostream &o, Foo &x) { o << x.val; return o; } int main() { Foo a(10); Bar b(20); a, b; cout << a; }This will output 30...
-
Whoops, this actually doesn't compile, because I forgot to put semicolons after the class declarations (I keep doing this constantly). But when you add them, it works. You'll also have to add
#include <iostream>
using namespace std;
-
I came up with one more idea: use boolean expression short-circuiting to code program logic. I.e. you want to open a COM port, if this fails, an LPT port, and if this fails too, a file. The functions return true if they succeed and false if they fail. If everything fails, you want the program to terminate:
OpenComPort() || OpenLptPort() || OpenFile(path) || TerminateProgram();
In PHP or Java, because of boolean short-circuiting, this would do the job. In C, particularly when not using any optimization, this would always terminate the program. If one is not familiar with the concept of short-circuiting, a line like this can be totally mind-blowing. It took me quite a while to figure out why
$db = new mysqli($host, $user, $pass, $dbname) or die('Connection failed');usually didn't kill the PHP script.
-
@ammoQ said:
If your code looks like this:
logger.log("foobar: found "+n+" records");
using StringBuilder would IMO mean over-engineering.
mine actually looks like logger.log(string.Format("foobar: found {0} records", n));
Well, that's not true, since I use log4net it looks like:
if(log.IsInfoEnabled)
log.InfoFormat("found {0} records", n);But, well, you get the idea. Personal preference, but I like how the string.format method reads much more than the concatenated approach.
-
@Tweenk said:
I came up with one more idea: use boolean expression short-circuiting to code program logic. I.e. you want to open a COM port, if this fails, an LPT port, and if this fails too, a file. The functions return true if they succeed and false if they fail. If everything fails, you want the program to terminate:
OpenComPort() || OpenLptPort() || OpenFile(path) || TerminateProgram();
In PHP or Java, because of boolean short-circuiting, this would do the job. In C, particularly when not using any optimization, this would always terminate the program. If one is not familiar with the concept of short-circuiting, a line like this can be totally mind-blowing. It took me quite a while to figure out why
$db = new mysqli($host, $user, $pass, $dbname) or die('Connection failed');usually didn't kill the PHP script.
No, C's short circuit evaluation is just as carefully defined as PHP's or Perl's -- possibly more so, since you can determine in advance what values an expression can return. The following code will always behave:
@ss.c said:
#include <stdio.h>
int a () {
printf("In a()\n");
return 0;
}
int b () {
printf("In b()\n");
return !0;
}
int main () {
printf ("a() || b():\n");
a () || b ();
printf ("b() || a():\n");
b () || a ();
}$ gcc -o ss ss.c -std=c99 -W -Wall -pedantic
$ ./ss
a() || b():
In a()
In b()
b() || a():
In b()
$The only catch is that both sides of an || or && expression must evaluate to integral types, which rules out using "|| exit(1)" on failure (exit's prototype is void exit(int);).
-
@webzter said:
mine actually looks like logger.log(string.Format("foobar: found {0} records", n));
Well, that's not true, since I use log4net it looks like:
if(log.IsInfoEnabled)
log.InfoFormat("found {0} records", n);But, well, you get the idea. Personal preference, but I like how the string.format method reads much more than the concatenated approach.
Well, the problem with string.format - compared to concatenation - is when you need a lot of variable parts, say more than 5 or so - like in
log.InfoFormat("something strange happened, foo={0}, bar={1}, mir={2}, doch={3}, wurscht={4}, bla={5}, blubb={6}", foo, bar, mir, wurscht, doch, bla, blubb);
- it's not easy to see at first glance which is which; if it was
log.InfoFormat("something strange happened, foo="+foo+", bar="+bar+", mir="+mir+", doch="+wurscht+", wurscht="+doch+", bla="+bla+", blubb=+"blubb);
it's IMO a bit easier to see why the results are so strange ;-)
Anyway, that's not to say that you should not use string.format or similar methods. The big advantage of string.format and similar methods is when it comes to i18n, they are much easier to translate as a whole.
-
@Tweenk said:
I think in regular C++ there are many more ways to create atrocities. Perhaps the most cruel one arises from the fact that the comma is also an operator, and therefore we can have an OVERLOADED COMMA OPERATOR!! <insert mad laugh>
Leave the comma operator alone! It has its uses, just look at boost::lambda or boost::assign (well, I'm sure at least one of them overloads it for good reason).
There has to be as much rope as nessesary to shoot oneself in the foot (and then some). And if one can't use a real gun, one better use a plastic one or pay the price! >:E
