What are some common mistakes made by .NET developers, and how can we avoid them?
For example, trying to open a file without checking whether or not it exists, or catching an error unnecessarily.
Please look in to the list before posting new
Please justify your answer as well, if applicable and give examples.
Equals, but not GetHashCode (nor != / ==)for(...) vs foreach(...) etc)using your IDisposable objectsThe most common error I make is starting to code without thinking first. I still catch myself doing it from time to time...
Does happen when I work outside the .net framework, too.
Another bad habit (which I successfully dropped) is swallowing exceptions:
try
{
//something
}
catch
{
// do nothing
}
Understanding the pitfalls of exception handling took me some effort, but was worth the time I spent on it.
throw ex;
Instead of
throw;
The first example will reset the stack trace to the point of the throw, whereas the latter will maintain the original stack. This is crucial for debugging.
throw ex;
Is never right when being used to re-throw an exception.
1 - Some of us don't use using. Use using where ever possible
using (StreamReader reader=new StreamReader(file))
{
//your code here
}
2 - Forgetting to check if something is null, instead of trying to catch a null condition when an exception occurs
//This might throw an exception
string val=obj.Value.DomeSomething()
//Better, check for null
if (null!=obj.Value)) {
// Do your stuff here
}
3- Forgetting to check for null after a runtime type cast
MyType t= x as MyType;
if (null!=t) {
//Do stuff here
}
4- Where ever you are allocating something in try block, make sure you've a finally block to release stuff.
I am sure that I have more. These are more like my current "Top Pet Peeves":
Not properly disposing of disposable resources. The using() keyword should be used with every object that implements IDisposable. Considering the code compiles to the equivalent of a try / finally that properly disposes of the object provides cleaner and safer code.
Catching Exception instead of a specific exception. Not only that, but seeing code where every single method has the entire body inside of a big try / catch. You should only ever catch exceptions that you can handle and are expecting. Add a top level handler to catch unhandled exceptions.
Seeing exceptions used to control program flow. If an exception is thrown, it should not be swallowed or used to initiate another set of logic that would not happen for any other reason.
not using a using statement to ensure that Dispose() is called.
Deploying your ASP.NET applications to production with Debug="true" set in the web.config. The compiler can't do any optimizations, and batch build is disabled. When we used to debug performance problems, this was one main area we'd look at. Tess has a great article on it.
It's so common, there is a built-in command to the SOS extension to WinDBG. Just get a memory dump of an ASP.NET application and run:
!finddebugtrue
which would output something like:
0:016> !finddebugtrue Debug set to true for Runtime: 61b48dc, AppDomain: /MyDebugApplication Debug set to true for Runtime: 1f50e6d8, AppDomain: /MemoryIssues Total 16 HttpRuntime objects
Tess' article above has more examples.
Use this cast:
Tree tree = obj as Tree;
... only if the program logic is such that you anticipate obj may or may not be of type tree.
In the situation where you expect that obj will only ever be of type Tree, prefer this style of cast:
Tree tree = (Tree)obj;
Prefer a (TargetType) style cast unless you really do need to make use of the conditional functionality offered by an 'as' cast.
Note: be sure to follow an 'as' cast with an 'if' or other appropriate logic to ensure that if the result of the 'as' was null, an attempt won't be made to dereference it. This is a mistake:
Tree tree = obj as Tree;
tree.GrowBranch(); // Bad. Possible NullReference exception!
In this case, the programmer meant one of these:
// Expected obj always to be a tree
Tree tree = (Tree)obj;
tree.GrowBranch();
// Expected obj could be a tree or could be something else
Tree tree = obj as Tree;
if( tree != null )
{
tree.GrowBranch();
}
Some people believe that...
Tree tree = (Tree)obj;
...is bad because it may throw an exception if the prerequisite that obj is a Tree isn't met. It's not bad though, because it will throw an InvalidCast exception. That's the right sort of exception and is thrown at the right time.
The NullReference exception that occurred after the 'as' cast in the first GrowTree() example gets thrown:
These two reasons make it more difficult to debug and determine what the real problem was.
The performance if these two types of cast is similar. It is true that a (TargetType) style cast throws an exception if the cast fails. However, this is not a problem that would affect performance. The reason is that we use a (TargetType) style cast only when we expect the cast will always succeed. So, no exception should ever be thrown! If an exception does get thrown, then there is a problem in the logic/design of the code. Fixing a problem like this by changing the (TargetType) cast into an 'as' style cast is probably wrong as it will probably just mask the real cause of the problem.
Using the 'as' cast instead of the (TargetType) cast because you think it looks prettier is not a good reason for writing incorrect code.
Writing:
Tree tree = obj as Tree;
if( tree != null )
{
tree.GrowBranch();
}
every time you need a cast, "just to be on the safe side" is absurd. You have to stop somewhere, otherwise one day you'll find yourself writing:
if( thisComputersPowerHasFailed )
{
SendEmailToAdministratorToSaySomethingHasGoneWrong();
}
Code like this introduces more and more conditional execution paths through your code. Every time you write some code to cope with a case that you don't expect should happen, you will increase the complexity of your program. Unnecessary complexity is just the kind of thing that causes bugs to slip in to code. The root causes of bugs will be tricky to find because they'll be hidden behind other unnecessary error handlers that try to hide or log the problem and carry on. A (TargetType) cast adheres to the generally good advice of writing code to fail-fast.
Not unhooking event handlers appropriately after wiring them.
This is because each event registration causes a reference to be created from the event source to the object/delegate that handles the event. These references can easily form part of a path of references from a root heap object to the handler.
Any object that has a path of references from the root heap object will not be garbage collected.
Locking on this is always a nice one. Not immediately fatal but a good way to get deadlocks.
One of the most dangerous pitfalls:
Creating a temp object, using its events by utilizing AddHandler (in VB) and then forgetting to remove those handlers. One would think that the object is collected by Garbage Collector when it goes out of scope, but it won't since there is still a pointer to that object (a function pointer) and GC won't clean it up.
You will also notice that the event handler hits many times. Once for every object you've created, used its events, and forgot to remove it. In addition to memory problems, this would cause your app to run slower and slower while it is working because the code in your handler would execute multiple times.
Just realized this problem because of performance issues of my app.
Raising an event without first checking if it's null: ie:
public event EventHandler SomethingHappened;
private void OnSomethingHappened()
{
SomethingHappened(this,new EventArgs()); //if no one hooked up to this event, it'll blow up
}
Change the name of a property without carefully checking if it is used in data binding.
Properties are used for databinding. Unfortunately the binding mechanism in Windows Forms and WPF use the property name as string. If you change the name of a property, you will not get any compiler error or warning, only a runtime error if you are lucky.
I always get hung up on this one.
string s = "Take this out";
s.Replace("this", "that"); //wrong
oooops didn't actually change s....
s = s.Replace("this", "that"); //correct
Its pretty easy to make that mistake.
Don't use "magic numbers" in your code.
Ex:
if(mode == 3) { ... }
else if(mode == 4) { ... }
Use Enumerations wherever possible, so the meaning, not the number, is exposed:
if(mode == MyEnum.ShowAllUsers) { ... }
else if(mode == MyEnum.ShowOnlyActiveUsers) { ... }
(I could have used a switch statement here, as well)
If you are going to be doing a large amount of string concatenation, use the System.Text.StringBuilder object.
Bad:
string s = "This ";
s += "is ";
s += "not ";
s += "the ";
s += "best ";
s += "way.";
Good:
StringBuilder sb = new StringBuilder();
sb.Append("This ");
sb.Append("is ");
sb.Append("much ");
sb.Append("better. ");
Like, trying to open a file without checking whether it exists ...
This is not necessarily a mistake. If you know the file ought to exist (e.g. a configuration file, or a file name obtained using an OpenFileDialog), it's often perfectly OK to just go ahead and open it, and let any exception propagate.
And checking for existence doesn't guarantee it will still exist when you try to open it.
It may make sense to check if you're opening a file in the presentation tier - where you can, for example tell the user the file doesn't exist.
But in the business tier, what are you going to do if the expected file doesn't exist?
Throw a FileNotFoundException? In which case you might as well just try to open the file.
Throw a custom exception? In which case callers will need to be aware that either your custom exception (for the common case) or a FileNotFoundException (if the file disappears between checking and attempting to open) - which potentially adds complexity.
Unnecessary initialization
DataTable foo = new DataTable(); // Initialization unnecessary
foo = FetchDataTableFromDatabase();
better:
DataTable foo = null;
foo = FetchDataTableFromDatabase();
also fine
DataTable foo = null;
try{
foo = FetchDataTableFromDatabase();
}
catch{
//do something
}
best
DataTable foo = FetchDataTableFromDatabase();
Avoid thinking you're a Hero and ask those around you for help!
Or doing this:
if ( SomeValue == null )
SomeObject.Value = null;
else
SomeObject.Value = SomeValue;
Instead of this:
SomeObject.Value = SomeValue ?? null;
EDIT: My point is, don't write three lines when one will do.
EDIT, from GordonG:
I think what he meant to say was...
Or doing this:
if ( SomeValue == null )
SomeObject.Value = OopsSomeValueWasNull;
else
SomeObject.Value = SomeValue;
Instead of this:
SomeObject.Value = SomeValue ?? OopsSomeValueWasNull;
I've caught myself a few times writing my getter and setter properties in C# incorrectly by referencing the name of the property in the get {} set {} blocks instead of the actual variable. Doing this causes an infinite loop due to the self-referential calls and eventually a StackoverflowException.
Example (Incorrect)
public int Property
{
get
{
return Property;
}
}
i = i++;
is not the same as
i++;
I found a great SO post about this, but this also does a decent job.
It's a dumb thing to do anyway, but the point is that it increments differently from c++.
Ah here is the SO one What’s the difference between X = X++; vs X++;?
Setting local objects to null will not free up any resources, so there is usually no reason to do it.
Use generics collections (List<T>) instead of ArrayList so that you can maintain type safety.
re:using
Don't unnecessarily nest using statements do this instead:
using (SQLconnection conn = new SQLConnection() )
using (SQLCommand cmd = new SQLCommand("select 1", conn)) // no need to nest
{
conn.open()
using (SqlDatareader dr = cmd.ExecuteReader()) //nessesary nest
{
//dr.read()
}
}
Use FxCop to pick up on common coding mistakes. Some of the things it picks up on are a bit trivial, but it has helped us pick up a number of bugs which might otherwise have been missed. Run it from Visual Studio, Analyze->Run Code Analysis for ..., or be really good and set it up to run every time you do a build in the Code Analysis section of the project properties.
As some have mentioned swallowing exceptions is a really bad habit. Another thing that I see people do all the time that's quite similar to swallowing exceptions is redundant null checks.
Let's say that we have a method that returns an object and we it should never return null a lot of times you'll see this.
var o = this.Foo();
if (o != null)
{
o.Bar();
}
// more code here...
Really if the method Foo returns null this is exceptional behavior and an exception should be thrown, so either in an else block throw an appropriate exception or just remove the if-statement and allow for the NullReferenceException to be thrown.
Oh, I forgot my number one pet peeve, over specification of input parameters. Let's say we have a method that takes a collection of some type, always allow the least specific type of collection needed by the method.
This is what you see A LOT:
public void Foo(List<Bar> bars)
{
foreach(var b in bars)
{
// do something with the bar...
}
}
As you see the method does nothing but loops through the list, so requiring a list is an over specification, all you need is something you can loop through, ie. an IEnumerable<Bar>.
This is a lot nicer to the caller:
public void Foo(IEnumerable<Bar> bars) ...
If the method requires to know the count of the bars collection use ICollection<Bar> instead, or maybe you need to access bars by index, then use IList<Bar> but always the least specific sub type needed.
using leading (or trailing) underscores to denote member data (or anything else)
class HowToMakePeopleHateYou
{
string _doShit;
double _likeThis;
}
There are a total of probably 10 people in the world who prefer to read code formatted that way. Chances are, your co-workers will not be among them.
Same goes for Hungarian notation, or any other punctuation meant to imply type or role of a variable.
Not using ReSharper! (Or another code analysis tool - but R# is the best.)
I'm surprised nobody has mentioned it yet, because it automatically picks up many of the mistakes mentioned in other answers.
Violating standards or conventions without knowing why they are there, or worse, refuse to even acknowledge their value.
It makes their code hard to read, hard to re-use.
1. RAII (resource acquisition is initialization)
A stupid name for a great idea. In C++, constructors are mirrored by destructors. After some serious internal and external lobbying right before C# was released, MS added the using statement, providing at least minimal support for this idea, though there is more they could do. But the usefulness of RAII is still not widely grasped. It's sort of true to say it cleans up "unmanaged resources", but think about what that means: anything other than memory. Think of all the places in your code where you modify state, and later want to put it back again.
Simple example - an Undo system. You want to support "batch" undo transactions, in which several actions get bound up into a single one. The application would do this:
undoSystem.BeginTransaction();
// do stuff, add several undo actions to undoSystem
undoSystem.EndTransaction().
The point is, EndTransaction MUST be called, however we exit the function, to restore the system to the state we found it in. You should at least use try/finally - but why not follow a pattern consistent with the language? Make BeginTransaction return an object:
public class UndoTransaction : IDisposable
{
public void Dispose()
{
// equivalent to EndTransaction
}
}
Now the application code can just do this:
using (undoSystem.BeginTransaction())
{
// do stuff, add several undo actions to undoSystem
}
Now there is no need to correctly figure out which method is the "ender" for the "beginner" of the state (which in some situations would not be as obvious as in this example). And ask yourself - would it make much sense for UndoTransaction to have a finalizer as well? Absolutely NOT. Finalizers cannot safely call on to managed objects, and they run in a different thread. The one thing they are useful for (calling an interop API to dispose of a Win32 handle) is now done much more easily by using SafeHandle.
Unfortunately the internet and older books are riddled with advice about how IDisposable implies the need for a finalizer. Ignore them. And also not really explaining the implications of "unmanaged resources" - anything about the state of your program can be regarded as an "unmanaged resource". By taking advantage of IDisposable/using, you can apply a consistent coding style to deal with states that change in line with the method call stack. Which brings us to...
2. Exception Safety.
There are some operations that do not throw. Assignment cannot be redefined in C#, so:
x = y;
Assuming x and y are fields or variables of the same type, that will never, ever throw (except under truly bizarre circumstances where you can no longer rely on anything working). How reassuring is that?! But also useful. Think of how, often, one of your methods will update the state of the class it belongs to. Sometimes it will modify two or three (or more) private fields.
What if an exception is thrown at some point during this multiple-update of state? What state is your object left in? Will one of the fields be updated, but not the other two? And does the resulting state make any sense? (does it "satisfy the class invariant"?) Or will it later cause your code to get confused and cause further damage?
The solution is to figure out what the changes need to be, before doing anything to update your fields. Then when you have all the answers ready, do the assignments - safe in the knowledge that assignments never throw.
Again, because of GC, C# programmers have been encouraged to think that this is a C++-specific problem. It's true that exception safety (and RAII) are commonly spoken of in terms of deleting memory allocations, but that is just one example (it happens to be very important in C++). The truth is, exception safety is an issue that concerns any program that has non-trivial modifiable state in it, which is most programs.
Another issue with exceptions is that they are just as much a part of the "interface" exposed by a method as are the parameters and the return value. We are encouraged (by some of the people answering this question) to catch specific exceptions instead of just Exception itself:
try
{
funkyObect.GetFunky();
}
catch (SocketException x)
{
}
How do you know that GetFunky throws SocketException? Either documentation, or trial and error. What if the author of that method later changes it so it doesn't use sockets, so it throws something else? Now you're catching the wrong thing. No warning from the compiler.
Compare with this cautionary tale:
IEnumerable<int> sequenceInts = funkyObject.GetInts();
// I found out in the debugger that it's really a list:
List<int> listInts = (List<int>)sequenceInts;
Very clever, until the author of GetInts changes it to use yield return instead of returning List<int>.
The moral is that you shouldn't rely on undocumented, untyped coincidences, you shouldn't sniff out the internals of a method you are calling. You should respect information hiding. But this applies to exceptions as well. If a method allows a huge variety of exceptions to leak out of it, then it has a very, very complicated interface, which its author probably didn't intend for you to be reliant on. It's not really any of your business how a method works internally.
This is all partly the fault of lazy library authors. When writing a nice clean modular library, consider defining your own exception type(s). Make sure that your library's methods ONLY throw your approprate exception types and document this fact. Your library methods' code will look like this:
try
{
// do all kinds of weird stuff with sockets, databases, web services etc.
}
catch (Exception x) // but see note below
{
throw new FunkyException("Something descriptive", x);
}
I call this normalizing the exceptions. Note that by passing x into the constructor of FunkyException, we cause it to become the InnerException. This preserves complete stack trace information for logging/debugging purposes. Also note that this contradicts the advice given by several other answers to this question (including the highest rated answer), and also many blog posts on this subject. But there it is; I think those people are dead wrong. Exceptions are part of the visible interface of a method, and it is just as important to control that aspect of the interface as it is to specify the type of the parameters and return values.
And when catching exceptions thrown by a badly written or badly documented method (one that may or may not throw all manner of exception types - who knows?) I would advise that you do NOT catch whatever specific exception types it throws, discovered by trial and error in the debugger. Instead, just catch Exception - wherever you need to in order to ensure the exception safety of your program's state. That way, you are not becoming dependent on undocumented or coincidental facts about the internals of other modules.
But...
Unfortunately catching (Exception x) is a really bad idea until CLR 4.0 comes along. Even then, it still won't be ideal, though not as bad as it is today. And yet, it has long been advised by the Exception Handling block of Microsoft's Enterprise Library!
For the details, see:
In short - if you catch all exceptions, you also catch fatal exceptions (ones that you want to cause your program to stop and capture a stack trace or a mini dump). If your program attempts to limp along after such an exception, it is now running in an unknown state and could do all kinds of damage.
Reponses to several comments from P Daddy:
"Your advice to ignore the conventions prescribed to by the majority of the industry, as well as Microsoft themselves..."
But I'm not advising that at all. The official advice on Dispose/finalizers used to be wrong but has since been corrected, so that now I'm in agreement with the majority opinion (but at the same time this demonstrates that majority opinion can be wrong at any given time). And the technique of wrapping exceptions is widely used by libraries from Microsoft and 3rd parties. The InnerException property was added for precisely this purpose - why else would it be there?
"IDisposable is not RAII... the using statement, as convenient as it is, is not meant as a generic scope guard..."
And yet it cannot help but be a generic scope guard. Destructors in C++ were not originally intended as a generic scope guard, but merely to allow cleanup of memory to be customised. The more general applicability of RAII was discovered later. Read up on how local instances with destructors are implemented in C++/CLI - they generate basically the same IL as a using statement. The two things are semantically identical. This is why there is a rich history of solid practise in C++ that is directly applicable to C#, which the community can only benefit from learning about.
"Your Begin/End Transaction model seems to be missing a rollback..."
I used it as an example of some thing with on/off state. Yes, in reality transactional systems usually have two exit routes, so it's a simplified example. Even then, RAII is still cleaner than try/finally, because we can make commit require an explicit call but make rollback be the default, ensure that it always happens if there is not a commit:
using (var transaction = undoSystem.BeginTransaction())
{
// perform multiple steps...
// only if we get here without throwing do we commit:
transaction.Commit();
}
The Commit method stops the rollback from happening on Dispose. Not having to handle both kinds of exit explicitly means that I remove a bit of noise from my code, and I automatically guarantee from the moment I start the transaction that exactly one of rollback and commit will occur by the time I exit the using block.
Case Study: Iterators
The IEnumerable<T> interface inherits IDisposable. If the implementation needs to do something interesting in its Dispose method, does that imply that it should also have a finalizer, to protect itself from users who do not call Dispose?
For an example, look at the most widely used (in modern C#) way of implementing IEnumerable<T>.
When you write an iterator (a function returning IEnumerable<T> and utilizing yield return or yield break), the compiler writes a class for you which takes care of implementing IEnumerable<T>. It does do something important in its Dispose, and yet it does not have a finalizer.
The reason is simple. The Dispose method executes any outstanding finally blocks in the iterator code. The language implementors realised that it would be better for the finally block to never run than for it to run on the finalizer thread. This would have required anything called from finally blocks in iterators to be thread safe!
Fortunately, most clients of IEnumerable<T> use foreach, which works exactly like a using statement - it calls Dispose for you. But that still leaves the cases where the client needs to directly control the enumeration. They have to remember to call Dispose. In the event that they don't, a finalizer cannot be assumed to be a safe fallback. So the compiler does not attempt to solve this problem by adding a finalizer.
Ultimately, this is just one (very widely used) example that demonstrates that there is a class of cleanup problems for which lazy cleanup (GC, finalizer thread) is not applicable. This is why using/IDisposable was added to the language - to provide a purely deterministic cleanup pattern - and why it is useful in its own right in situations where a finalizer would be the wrong choice.
This is not to say that you must never add a finalizer to something that is disposable, just that finalizers are only appropriate in a subset of cases.
One common mistake is using
if (obj is SomeType) {
((SomeType)obj).SomeMethod();
}
instead of
SomeType tempVar = obj as SomeType;
if (tempVar != null) {
tempVar.SomeMethod();
}
The latter does not double the cast (which does occur in the first snippet and means a slight perfomance hit).
Also don't forget all the common code smells:
Referencing constants across assemblies, that may not get updated together.
Here is an article i wrote in 2007, pasted whole sale
Referencing Constants
We all know the classic lesson from school. Never use “magic numbers” in your code. Always define a constant and use it throughout. Not only does it give it contextual meaning, it also makes it easy to alter the value only at one place in the future. Sweet deal huh? Well, maybe not as much as one might think. There is a subtle issue with the use of constants that perhaps not everybody is aware of. Let’s do something practical to sink the idea; go ahead and open up Visual Studio:
Those who play WOW know that the maximum attainable player level used to be 60. So let’s create a PlayerLimits class in ConstantLibrary
namespace ConstantExample
{
public class PlayerLimits
{
public const int MaxLevel = 60;
}
}
Now, in ConstantDependent, 1. Use Form1 2. Put in a button btnMaxLevel 3. Put in a label lblMaxLevel 4. Set the btnMaxLevel click event to
private void btnMaxLevel_Click(object sender, EventArgs e)
{
this.lblMaxLevel.Text = PlayerLimits.MaxLevel.ToString();
}
Build and run the solution. When you click the button, 60 appear. Now,
It remains at 60. Oops. What is happening here?
To be specific, it would be IL_0007 in the sample below.
.method private hidebysig instance void btnMaxLevel_Click(object sender, class [mscorlib]System.EventArgs e) cil managed
{
// Code size 24 (0x18)
.maxstack 2
.locals init ([0] int32 CS$0$0000)
IL_0000: nop
IL_0001: ldarg.0
IL_0002: ldfld class [System.Windows.Forms]System.Windows.Forms.Label ConstantExample.Form1::lblMaxLevel
IL_0007: ldc.i4.s 60
IL_0009: stloc.0
IL_000a: ldloca.s CS$0$0000
IL_000c: call instance string [mscorlib]System.Int32::ToString()
IL_0011: callvirt instance void [System.Windows.Forms System.Windows.Forms.Control::set_Text(string)
IL_0016: nop
IL_0017: ret
} // end of method Form1::btnMaxLevel_Click
The IL code is using the literal integer value of 60. Ouch. What the C# compiler has done is to inline the constant value literally into the client assembly. If you are in one of those environments where you are only allowed to promote changed assemblies into UAT or production environment, and you thought you could alter just an assembly with modified constants, well, we all thought wrong.
Recommendation: Use constants only within an assembly. If they are placed in some other assembly, make sure they get compiled together and promoted together, even when the client assembly has no change in code. If you can guarantee the constants never change values, then power to you. Otherwise, use static read-only values for dynamic referencing. The following snippet will “propagate” the correct value to the client assembly even if it wasn’t compiled together.
public class RaidLimits
{
public static readonly int MaxPlayers = 25;
}
Not testing!!! This is the biggest mistake I made in the past. Glad I'm finally turning around. Due to testing my code is better, more logical and all of it is being made easier to maintain. Also, I noticed my speed in development went up as well. Not testing is the biggest mistake you can make imo ...
Using string literals repeatedly instead of string constants.
Accessing a single list item repeatedly instead of getting it once and assigning it to a local variable.
Using less efficient string comparisons like string1==string2 instead of more efficient ones like string.Equals, StringComparer.
Always making classes and methods public even for those that should be internal to a library or a class.
Assert.AreEqual(0, resultOfCalculation);
Comparisons with 0 aren't a good way to go, especially in engineering - in a complex calculation, the end result might diverge by at least double.Epsilon and typically a lot more due to all sorts of precision loss.
This is why some test frameworks overload the AreEqual() method, allowing the user to specify tolerance values:
Assert.AreEqual(expectedResult, actualResult, tolerance);
Not quite a language thing, and perhaps arguably a "mistake" but...Deploying web-based apps without using HTTP Compression (GZip, Deflate, etc.) There rarely a good reason why you should not use HTTP Compression...
Also, not checking Page.IsValid() to ensure server side validation is done.
Putting bad execution code in Get accessors
If you have code that modifies an objects state in its Get accessor, then examining that property in the debugger, which causes the code to execute, can alter the state of your object.
For instance, if you have the following code in a class...
private bool myFlag = false;
public string myString
{
get
{
myFlag = true;
return "test";
}
}
The first time you run into this in the Debugger, myFlag will show as having a value of false, and myString will show as having a value of "test". If you hover over myFlag, however, you will see that its value is now true. The debugger's display of the property value has changed the state of your object.
Adding threads to an app without knowing the basics of writing threaded apps.
Your class doesn't need a Finalizer just because it implements IDisposable!
You can implement IDisposable to give your class the ability to call Dispose on any owned composite instances, but a finalizer should only be implemented on a class that directly owns unmanaged resources.
Any compositely owned instances that own unmanaged resources will have their own finalizers.
What I really hate is when programmers ignore compiler warnings and just leave them in the code.
If your project ends up with 100 compiler warnings that you consider "okay to live with" when the 101st compiler warning appears that you might not be happy with you are very unlikely to spot it, you're then likely to be introducing unexpected behaviour.
On a similar line, I also hate it when people change source code in a way that causes it to break unit tests and then don't fix the source code or the test so that they pass. I've been working on a solution that has had 9 broken test cases new for the past 3 weeks and it is driving me mad! Whenever I break a unit test it is harder for me to find what I have broken.
In many cases I've seen this scenario:
IDbConnection conn = null;
try
{
conn = new SqlConnection("SomeConnString");
conn.Open();
}
finally
{
// What if the constructor failed?
conn.Close();
}
If an exception originated in the constructor of SqlConnection class, the variable would never be assigned, which will cause a System.NullReferenceException in the finally in the Close method never being called, resulting in a memory leak.
Generally, it is always a good idea to be as defensive as possible in the catch and finally blocks, since that code is critical and simply can't fail:
IDbConnection conn = null;
try
{
conn = new SqlConnection("SomeConnString");
conn.Open();
}
finally
{
if (conn != null)
{
conn.Close();
}
}
I would say mine are more general, as I deal with good architecture more than coding nick nacks:
String concatenation, already mentioned, is a big no no for me. Another is try ... catch. If you are not catching, use try finally. Just as a hands up for noobs, these are functionally equivalent:
using(SqlConnection conn = new SqlConnection(connString))
{
conn.Open();
}
AND
try
{
conn.Open();
}
finally
{
conn.Dispose();
}
One big one I have seen over and over again, with ASP.NET, is writing ASP.NET like ASP. The same can be said for VB.NET devs who moved over from VB. Binding = good; looping to Response stream = bad.
What else>? Oh, you can run through almost any sample on the web and see some pretty bad examples of proper architecture. I do not denigrate the writers, as they are not there to illustrate architecture, but to show how to solve one problem, but still ...
I saw this one recently...
public void MyMethod()
{
if (this == null)
{
// some kind of bizarre error handling...
}
....
}
He insisted that you should always check if what you're trying to look at is null before using it. My claim that you couldn't actually be in this method if "this" was null fell on deaf ears.
If you know in advance the size of collection you are about to fill, reserve the space when creating your collection.
List<Person> persons = new List<Person>(listBox.Items.Count); // Reserve
foreach (ListBoxItem lbi in listBox.Items)
{
persons.Add(lbi.Tag as Person); // No reallocation
}
For very large list, not reserving the space causes the collection to be re-allocated over and over (at each power of two).
Another tip: When adding a lot of items to a collection, it's more efficient to use one AddRange instead of a sequence of Add. This is especially true with observed collections like the Items collection on a ListView.
foreach (string line in File.ReadAllLines(fileName))
{
// Not the best: there might be overhead when the collection changes
// and multiple reallocations
listBox.Items.Add(line);
}
// Much faster: Single call, minimal overhead, and only one
// potential reallocation
listBox.Items.AddRange(File.ReadAllLines(fileName));
Calling GC.Collect().
It is a rare case when we need to interfere with the Garbage Collector work.
A common error when trying to create a central exception handler on winforms:
try
{
Application.Run(someForm)
}
catch (Exception ex)
{
//this won't catch your winforms exceptions
//(even if inside visual studio it does)
}
You should use this for the behaviour you want:
// Add the event handler for handling UI thread exceptions to the event.
Application.ThreadException += someThreadExceptionEventHandler;
// Set the unhandled exception mode to force all Windows Forms errors to go through
// our handler.
Application.SetUnhandledExceptionMode(UnhandledExceptionMode.CatchException);
I am a C++ programmer and there was a time when started to code in C# without doing any reading on the language, here are some of the mistakes I did.
Writing a method that write\read your object state to a file instead of using XML (or binary) serializers.
Applying the observer pattern instead of using the built in Events.
Applying the Iterator pattern instead of using enumerators and the IEnumerable interface.
Writing a clone method to classes without implementing the IClonable interface.
Not using the "Using" keyword on IDisposable objects.
Some realy strange things with strings which I can't recall now. (Strings in C# work a bit different then c++)
Not using flagged enum, where actually we need to.
public enum Hobby {
None,
Reading,
Cooking,
Cricket,
All
}
public class Student {
public List<Hobbies> Hobbies{get;set;}
//Bad: we have to make it a collection, since one student may have more than one hobby.
}
Better Idea.
[Flag]
public enum Hobby {
None=0,
Reading=1,
Cooking=2,
Cricket=4,
All=7
}
public class Student {
public Hobby Hobby {get;set;};
//Now, we can assign multiple values to it, eg: student.Hobby= Hobby.Reading | Hobby.Cooking;
}
For more info: http://msdn.microsoft.com/en-us/library/cc138362.aspx
I saw this one before: a programmer is assigned a 'feature' that results in an exception being thrown inside a catch. He proceeds by implementing another catch, that does nothing:
Original code:
try
{
// Do something
// Cause an exception
}
catch(Exception e)
{
Logger.Log(e.Message); // throws
}
Fixed code:
try
{
// Do something
// Cause an exception
}
catch(Exception e)
{
try
{
Logger.Log(e.Message); // throws
}
catch { }
}
We never saw the bug again, until customer looked at the final product. Need I say more.
I always get annoyed when I see these in web forms:
try
{
int result = int.Parse(Request.QueryString["pageid"]);
}
catch
{...
Instead of using
int result;
if (!int.TryParse(Request.QueryString["pageid"] , out result))
{
throw new ArgumentException....
}
Here is a good set of guidelines and best practices from Juval Lowy (founder of IDesign).
Most of them are presented as facts without justifications, but answers can be found in his book "Programming .NET components".
Although some statements can be considered as too strict and arguable I think it's worthwhile for everyone to run through his list and think about them.
Forgetting this:
DBNull.Value != null
From MSDN,
Do not confuse the notion of a null reference (Nothing in Visual Basic) in an object-oriented programming language with a DBNull object. In an object-oriented programming language, a null reference (Nothing in Visual Basic) means the absence of a reference to an object. DBNull represents an uninitialized variant or nonexistent database column.
The common mistake isn't necessarily thinking that DBNull.Value is a null reference. I think it's more common to forget that your DataTable or other database-sourced object contains DBNulls rather than null references.
Not writing defensive code that checks for nulls.
MyClass t = GetAnInstance();
t.SomeMetehodCall(); // may throw NullReferenceEx
// or worse...
SomeOtherFunction( t ); // may also throw NullReferenceEx
Debugging NullReferenceExceptions can be a frustrating time-killer in .NET development.
Forgetting that exception catch blocks are matched in the order defined and that more general exceptions will supersede more specific ones.
try
{
// some code that may throw...
}
catch( Exception x )
{
// catches all exceptions...
}
catch( NullReferenceException x )
{
// never reached because catch( Exception x ) is more general...
}
Resharper and FxCop can help avoid this error.
Failing to synchronize to the GUI thread from a different thread causing "Cross-Thread Operation not valid" Exception
This one sometimes gets us: in Linq, using First() instead of FirstOrDefault() on a IEnumerable that may be empty. Sometimes, it seems intuitive that First would return a null, rather than what it does with the exception throwing.
Check out this book: Framework Design Guidelines
it contains a lot of DOs and DO NOTs
Modifying a collection by iterating the collection
Contrived code sample:
List<int> myItems = new List<int>{20,25,9,14,50};
foreach(int item in myItems)
{
if (item < 10)
{
myItems.Remove(item);
}
}
If you run this code you'll get an exception thrown as soon as it loops around for the next item in the collection
The correct solution is to use a second list to hold the items you want to delete then
iterate that list i.e
List<int> myItems = new List<int>{20,25,9,14,50};
List<int> toRemove = new List<int>();
foreach(int item in myItems)
{
if (item < 10)
{
toRemove.Add(item);
}
}
foreach(int item in toRemove)
{
myItems.Remove(item);
}
Or if you're using C# 3.0 you can use the List.RemoveAll like this
myInts.RemoveAll(item => (item < 10));
I've run into a lot of code that doesn't make use of the TryParse functions in classes (like int.TryParse). It makes the code look a little neater and returns a bool result instead of throwing an exception in which you will have to try to catch.
Similar to this answer, but using String.Format.. It helps readability a lot..
Instead of this..
Console.WriteLine("A: " + 9 + "\nB: " + 34 + "\nC: " + someInt);
Use..
Console.WriteLine("A: {0}\nB: {1}\nC: {2}", 9, 34, someInt);
One horrible pitfall is a method calling itself when you meant to call an overload eg.
public static void MyMethod(int id, bool always, string filter)
{
// Do something
}
public static void MyMethod(int id, bool always)
{
MyMethod(id, always); // Ouch!
}
public static void MyMethod(int id)
{
MyMethod(id, false, null);
}
One of the worst thing even the experience programmers also doin is assigning db null values to variables without verifying it
-- missing db null verifications
always perform a check on db ops
(drOutput[iMainIdentifier] != Convert.DBNull) ? drOutput[iMainIdentifier].ToString() : null;
--Accessing the fields in nulled object without checking
--calling the nulled object without initializing it
--in ui apps, using non ui thread to update the UI
--using unavailable column names in the datareader getordinal() methods
--missing breaks in case staments..
--break ur loops carefully. there are scenarios we are looking for a specific item within a loop for further processing and letting the loop continue even the specified item already identified..
not catching any errors at all, or handling them wrong. I know some guys who will write whole programs and not catch any errors, then wonder why the s*** crashes. Maybe the bigger mistake is not knowing your development language thoroughly enough.
public DateTime IncrementDateByOneDay(DateTime date)
{
DateTime result = date;
result.AddDays(1); //ATTENTION
return result;
}
This did not change "result". Correct version:
public DateTime IncrementDateByOneDay(DateTime date)
{
DateTime result = date;
result = result.AddDays(1);
return result;
}
This is just an example of course. I found such bugs already inside other methods and it's really hard to find since it seems to be correct by just running over it.
Similar to this one, when defining a property
...
private string _MyString;
public string MyString
{
get
{
return MyString; //ATTENTION
}
set
{
...
}
}
At runtime this gives a nice crash. Of course this should be written as
private string _MyString;
public string MyString
{
get
{
return _MyString;
}
set
{
...
}
}
Boxing/Unboxing is one of the common oversights of new developers. This was a more serious problem in the early days of .Net (before generics) but it is still quite easy to create innocent-looking code which is massively unefficient due to implicit boxing/unboxing.
Risky code in the constructor. When I have to initialize an object with risky code (I. E. loading from XML), I usually have an Initialize() or Load() method, so I don't do it in the constructor, which can leave the object in an "incomplete" state. So instead of
try
{
MyObject o = new MyObject();
}
catch(Exception ex)
{
// handle exception
}
I do this:
MyObject o = new MyObject();
try
{
o.Initialize();
}
catch(Exception ex)
{
// handle exception
}
Checking a string variable if it is assigned. String object can be null so;
wrong!
if(someStringVariable.length > 0) {
//do you always get this far?
}
correct
if(!String.IsNullOrEmpty(someStringVariable)) {
//yeah! this one is better!
}
Not using Events, as a developer that came across from C++ (many years ago) I found that I programmed C++ in .NET instead of using the built in functionality.
In on of my first .NET project I've implemented the observer pattern using interface and inheritance instead of simply using events.
well, I went through all the answers, and they're all very important things to look out for, but I think the number one mistake of programmer is not documenting their code!!
it's not really a pure coding issue, but nevertheless I believe it is a most basic skill.
everyone makes mistakes, all of us write bugs. that's all human. but not commenting on your code, thereby making life easier on yourself, and more importantly for your fellow developer that will need to debug your code after you moved on, is the worst thing IMHO.
also, not using common design patterns (everyone knows singleton, but strategy, adapter, decorator, etc...), or not even knowing them, is something I see quite often.
Handling ThreadAbortException:
try
{
}
catch(ThreadAbortException ex)
{
}
It's hard (if not impossible) to write code that will always handle a ThreadAbortException gracefully. The exception can occur in the middle of whatever the thread happens to be doing, so some situations can be hard to handle.
For example, the exception can occur after you have created a FileStream object, but before the reference is assigned to a variable. That means that you have an object that should be disposed, but the only reference to it is lost on the stack somewhere...
An addition side note to catching the ThreadAbortException is when you call .Abort() on a thread, it throws an uncatchable ThreadAbortException. "Uncatchable" because even if you do catch it, it is automatically rethrown at the end of the 'catch' block, so you can't stop its propagation. This does, then, have the effect of terminating the thread (provided it doesn't go into a loop in the 'catch' clause).
Finding an alternative is going to save you a lot of time and headache's down the line.
Making sure to understand that strings are immutable in .NET.
So, whenever you do any operation on a string you get back a new string with the operation applied to the new string.
So if you have a lot of operations in a loop ( say appending values to a string ), it is instead recommended to use the StringBuilder object.
Use background workers to make sure your Windows Form does not freeze while executing a thread. End of.
string foo = GetString();
if (foo == null) throw ex1;
if (string.IsNullOrEmpty(foo)) throw ex2;
foo is checked twice for null.
The common coding mistake I find the amongst by people inexperienced with C# is that they do not account for the fact that string comparison is case-senstive (unless you explicitly specify otherwise) especially when the comparison value comes from a database:
var foo = dataRow["ColumnName"].ToString();
if (foo = "ABCDEF")
...
The most common reaction change I see is to convert both values to uppercase or lowercase instead of using the Equals method with a StringComparison enum like so:
//bad
if ( foo.ToUpper() == "ABCDEF")
//correct
if ( "ABCDEF".Equals(foo, StringComparison.CurrentCultureIgnoreCase )
...
( OrdinalIgnoreCase or InvariantCultureIgnoreCase...)
Obviously, there is also the problem with using string literals, but that has already been mentioned.
The most common mistake I see with people that write code that interacts with a database is not accounting for nulls.
This is one that's probably endemic to any language: Ignoring the rule of "Don't Repeat Yourself", or DRY. I sometimes wish it were possible to administratively disable an IDE's paste functionality beyond a certain clip size.
I've seen (and cleaned up) way too many instances where somebody needed a method "just like this here one", only slightly different. All too often, the method in question is about fifty lines long, and the change they want to make is to some literal magic number that's peppered throughout, so they make a copy of the method, change all the appearances of the magic literal (or whatever trivial change they needed), and call it good.
Sometimes, they don't even commit this travesty at the method level! I once found a pile of cases in a switch where each of the case bodies were so nearly identical (and loooonng!), the "programmer" had to scope each case to avoid renaming his temporary variables. I confronted him about this, and he thought he'd been incredibly clever. Unfortunately, in this particular situation, it would've been impolitic to burst his bubble, but it was tempting nonetheless.
So, people, please, THINK before you insert! (Hmm... That's good advice in life as well as coding, isn't it? ;-) If there's a method that does nearly what you need to accomplish, rework it to additionally support your case and reuse, recycle, rejoice! If it's part of a larger method, extract the code into its own method and have the original method call it! Code may be good, but more code is NOT better!
Ahem. *sigh* I feel much better now. Thank you.
Be careful when calling calling exception handling methods in your catch statement, where the exception handling code throws and exception itself. You will be caught in an continuous loop. I found this issue a number of times through the years.
Here my favorite pet peeves:
a) Writing C++ style or (even worse) Java style programs
b) Premature optimization
c) Classes with more than one responsibility
d) Ignorance towards design patterns
e) Ignorance towards language and CLR features
f) Use of P/Invoke because of ignorance instead of necessity
g) "Yoda me desire like": if (5 == x)
h) "Walking like an egyptian" ({ on the same line as the introducing statement, like those Java people do it for some reason)
i) No curly braces - code without that is UNREADABLE
j) Procedural code, especially methods with more than 3 lines of code
k) Ignorance towards modern, functional language features
l) Copy & Paste
m) Code in places where it doesn't belong
in C#, a lot of people are still using the "" to declare an empty string. I consider this as a bad programming practice. people should use string.Empty to strictly specify that the string is empty and not a empty quote that looks like somebody accidentally deleted the string.
example: BAD
string someString = "";
example: GOOD
string someString = string.Empty;
Another bad practice is with poor use of StringBuilders. Some people append a break line by applying a new line code inside the string when they can actually just use .AppendLine.
example: BAD
StringBuilder sb = new StringBuilder();
sb.Append("first line \n");
sb.Append("second line \n");
or
StringBuilder sb = new StringBuilder();
sb.Append("first line <br/>");
sb.Append("second line <br/>");
example: GOOD
StringBuilder sb = new StringBuilder();
sb.AppendLine("first line");
sb.AppendLine("second line");
or
StringBuilder sb = new StringBuilder();
sb.AppendLine("first line");
sb.AppendLine("second line");
Response.Write(sb.ToString().Replace(Environment.NewLine, "<br/>"));
I see codes that use unnecessary methods calls. I've done that before, but with a little care, i think we can make it better.
here is a little example;
public string TestToString(int param1, int param2, object param3) {
string result = String.Format("{0}-{1}-{3}", param1.ToString(), param2, param3.ToString());
return result;
}
Calling ToString() is really not necessary for param3, it is clear you can see with a help of ildasm.
.method public hidebysig instance string
TestToString(int32 param1,
int32 param2,
object param3) cil managed
{
// Code size 37 (0x25)
.maxstack 4
.locals init (string V_0,
string V_1)
IL_0000: nop
IL_0001: ldstr "{0}-{1}-{3}"
IL_0006: ldarga.s param1
IL_0008: call instance string [mscorlib]System.Int32::ToString()
IL_000d: ldarg.2
IL_000e: box [mscorlib]System.Int32
IL_0013: ldarg.3
IL_0014: callvirt instance string [mscorlib]System.Object::ToString()
IL_0019: call string [mscorlib]System.String::Format(string,
object,
object,
object)
IL_001e: stloc.0
IL_001f: ldloc.0
IL_0020: stloc.1
IL_0021: br.s IL_0023
IL_0023: ldloc.1
IL_0024: ret
} // end of method Test::TestToString
And for param1 is not that neccessary at all, let suppose it is a type of decimal, still you don't have to call ToString if you don't need to display culture specific representation.
In any case boxing will work faster than calling ToString() method. It matters if you call that kind of methods soo often in short period.
Overengeneering...
http://en.wikipedia.org/wiki/Overengineering
Starting with interfaces and models and concepts and then stuck because I didn't noticed this and that, then changing interfaces and classes and change code and then really stuck...
I've learned: build systems from a ground up, not the opposite :)
cheers
class Foo : IDisposable
{
public int Denominator { private get; set; }
public int DoDivision(int x)
{
return x / Denominator;
}
public void Dispose()
{
Console.WriteLine("I do nothing in Dispose method.");
}
}
The following is not wrong but...
class Program
{
static void Main(string[] args)
{
try
{
using (Foo f = new Foo())
{
f.Denominator = 0;
f.DoDivision(1);
}
}
catch (Exception ex)
{
Console.WriteLine(ex.Message);
}
}
}
why not do like so:
class Program
{
static void Main(string[] args)
{
Foo f = null;
try
{
f = new Foo();
f.Denominator = 0;
f.DoDivision(1);
}
catch (Exception ex)
{
Console.WriteLine(ex.Message);
}
finally
{
if (f != null)
f.Dispose();
}
}
}
Misconception about static fields.
If two instances of an assembly running on two separate processes, the static fields are no longer related. Modification on static fields in the first process will not affect static fields in the other process.
My pitfall when I learnt C#, I get provocated with the statement that "all instances of the class will share the same static fields". This statement does not emphasize that it is only true if all instances are in the same process.
Worst programming mistake that bothers me daily, especially in others code is not commenting on your code. Properly Commented Comments are your friend.
Not understanding memory managment and garbage collection
The pit fall is assuming that just because it is a managed environment, I don't have to know about memory management. That's a mistake. Not learning about these things will eventually lead to memory leaks.
This includes correct implementation of IDisposable and how to manage disposable objects correctly.
Here is one in asp.net.
<form id="form1" runat="server">
<asp:placeholder id="ph" runat="server">
<asp:TextBox id="tb1" ruant="server" Text="test" />
</asp:placeholder>
<asp:TextBox id="tb2" runat="server" Text="another" />
</form>
And in code behind he tries to do something to all TextBoxes.
foreach(TextBox tb in form1.Controls.OfType<TextBox>())
{
//here he can only get tb2
}
because .Controls returns the first-level child controls only (in this example will return ph and tb2. To get all child controls, you need a recursion:
public static IEnumerable<T> GetChildControls<T>(this Control control) where T : Control
{
var children = control.Controls.OfType<T>();
return children.SelectMany(c => GetChildControls<T>(c)).Concat(children);
}
Believing that enums are exhaustive:
enum YesNo
{
Yes,
No
}
string GetColloquial(YesNo value)
{
switch (value)
{
case YesNo.Yes:
return "Yeah";
case YesNo.No:
return "Nope";
default:
return "This will never happen"; //oh yes it will!
}
}
Enums are internally implemented as integers, so you can still do things like this:
Console.WriteLine(GetColloquial((YesNo)5));
This one can be dangerous, so watch out!
Believing that you're improving array iterating loop performance by extracting the Length property ahead of time.
Surprisingly, this is actually slower:
int[] values = int[10000];
int length = values.Length;
for (int i = 0; i < length; i++)
values[i] = i;
Than this:
int[] values = int[10000];
for (int i = 0; i < values.Length; i++)
values[i] = i;
The .NET runtime injects a check each time you access a value in an array, to make sure that the index you've given it is >= 0 and < Length. This eliminates a whole slew of horrifying security flaws known as buffer overflows, and is one of the main selling points of so-called "managed languages" like C# or Java or Python over an unmanaged language like C/C++.
On the other hand, of course, this impedes performance to some degree (usually not much). .NET alleviates this by making an exception for certain cases. In loops like the one above, where the each value from [0] to [length-1] is being accessed, the check is ommitted, and the code run as fast as if it were an unmanaged language.
But this optimization can't be performed if Length is copied to a local variable first, because it has no easy way of knowing that your variable is correct.
doing
HyperLink_Back.NavigateUrl = Request.UrlReferrer.ToString();
without checking
Request.UrlReferrer != null
Avoid doing risky instantiation in a static field initializer
WRONG:
public class Whatever {
private static SomeExternalComObject externalObj = new SomeExternalComObject();
// ...
}
Slightly better:
public class Whatever {
private static SomeExternalComObject externalObj;
static Whatever() {
try {
externalObj = new SomeExternalComObject();
} catch(ComException ex) {
// backup strategy and/or logging and/or whatever else
}
}
}
Otherwise debugging will be a great pain in the ... neck.
Array of non-object cannot be cast to array of object
int[] data = { 1, 2, 3 };
object[] objs = data;//compile time error!!!
Returning null when an empty list/sequence/array would suffice.
HtmlAgilityPack's SelectNode function returns null instead of an empty list, so what I could have written simply as:
var links =
node.SelectNodes("//a")
.Select(n => n.Attributes["href"].Value);
Ends up having to be:
var linkNodes = node.SelectNodes("//a");
IEnumerable<string> links;
if (linkNodes != null)
links = linkNodes.Select(n.Attributes["href"].Value);
else
links = new string[] { };
Urrrghhhh!!!!1
Believing that casting a collection to IEnumerable makes it read-only.
GetReadOnlyList does not return a read-only collection. It can be written to by casting back to List:
public IEnumerable<int> GetReadOnlyList()
{
var list = new List<int> { 1, 2, 3 };
return list as IEnumerable<int>;
}
public void UhOh()
{
var list = GetReadOnlyList() as List<int>;
list.Add(4);
}
You can avoid this is by returning list.AsReadOnly() instead.
This one almost bit me good:
private const string connectionString
= Properties.Settings.Default.ConnectionString;
Since the const connectionString is resolved at compile time not runtime, I mistakenly believed that this was actually configurable.
What I needed was a readonly variable as such:
private readonly string connectionString
= Properties.Settings.Default.ConnectionString;
Which is instead, resolved at runtime. :)
I link Effective C# because I had read an item addressing why you should use readonly over const like a week before so it caused me to realize the error sooner. :)
Never make your Windows Forms controls public. Always keep them private and use properties if you need to.
public string Name
{
get
{
return textBoxName.Text;
}
}
and when you want to parse some text from Other form make as much as possible in that form to keep main form clear.
public string Command
{
get
{
return "#Wait(" + textBoxTime.Text + ")" + Environment.NewLine;
}
}