tags:

views:

502

answers:

5

When assigning a defualt default-value to a field (here false to a bool), fxcops says :

Resolution   : "'Bar.Bar()' initializes field 'Bar.foo' 
               of type 'bool' to false. Remove this initialization 
               because it will be done automatically by the runtime."

Now, I know that code as int a = 0 or bool ok = false is introducing some redundancy, but to me it seems a very, very good code-practice, one that my teachers insisted on righteously in my opinion.

Not only is the performance penalty very little, more important : relying on the default is relying on the knowledge of each programmer ever to use a piece of code, on every datatype that comes with a default. (DateTime ?)

Seriously, I think this is very strange : the very program that should protect you from making all too obvious mistakes, is suggesting here to make one, only for some increased performance? (we're talking about INITIALIZATION CODE here, only use one time!! Programmers who care that much, can ommit of course the initialization, (and should probably use C or assembler :-))

Is FXCop making an obvious mistake here, or is there more to it?

2 Updates :

  1. This is not just my opinion, but what I have been teached at university (belgium), not that I liketo use an argumentum ad verecundiam but just to show that it isn't just my opinion, and concerning that :

  2. My apologies, i just found this one : http://stackoverflow.com/questions/636102/should-i-always-ever-never-initialize-object-fields-to-default-values

+6  A: 

There can be significant performance benefits from this, in some cases. For details, see this CodeProject article.

The main issue is that it is unnecessary in C#. In C++, things are different, so many professors teach that you should always initialize. The way the objects are initialized has changed in .NET.

In .NET, objects are always initialized when constructed. If you add an initializer, it's possible (typical) that you cause a double initialization of your variables. This happens whether you initialize them in the constructor or inline.

In addition, since initialization is unnecessary in .NET (it always happens, even if you don't explicitly say to initialize to the default), adding an initializer suggests, from a readability standpoint, that you are trying to add code that has a function. Every piece of code should have a purpose, or be removed if unnecessary. The "extra" code, even if it was harmless, suggests that it is there for a reason, which reduces maintainability.

Reed Copsey
unnecessary for who or what? You of course code for persons to, for a machine comments are unnecessary too, this doesnot mean they are unnecessary perse
Peter
You can put it in as a comment, just as easily as a statement, and not create the same performance impacts. It's like having code that is unreachable - it may provide something to you, but I'd rather have it as a comment than leave unreachable code in my methods.
Reed Copsey
The difference with a comment, is when I say int i = 0; I want i to be zero, the statement is logical. Even when compiled with a future compiler that has other defaults (unlikely for int ofcourse)
Peter
And a question : if you want a date to be initialized on 1/1/1, do you omit this value because it's unnecessary, and do you seriousliy think this would be a good idea?
Peter
@Peter: You're guaranteed that an int will always be defaulted to 0 by the CLR spec - this isn't a compiler issue, but rather a runtime/platform issue. If you want a date to be initialized to a non-default value, by all means initialize it, but only if you're using a non-default value.
Reed Copsey
atReed (my at won't work for some reasen :))but with a date, 1/1/1 happens to be the default, ommiting that don't seem clear nor a good practice if you mean that (indeed historical) date.
Peter
@Peter: This is a private member variable. In that case (which is kind of an extreme case), I'd probably make a comment/remark that I did want that date, and still eliminate it. FxCop won't complain, though, if you specify the date as = new DateTime(1,1,0001);
Reed Copsey
...Since that's not the default constructor. It will only complain if you do date = new DateTime();
Reed Copsey
@Peter I see your point, but in this case, your default value should be a constant (or a static readonly value) to make it more explicit. int i = DEFAULT_WHATEVER; or DateTime d = DEFAULT_BIRTH_DATE; instead of int i = 0; and DateTime d = new DateTime(1, 0, 1);
Michael Meadows
atReed : The "extra" code, even if it was harmless, suggests that it is there for a reason, which reduces maintainability. --> it is there for a reason, for clarity of course, and for unforseen future uses.The widely unneeded accepted curly braces after an if are there for the same
Peter
reason `if (test) { doIt(); }` atMichael : tx
Peter
The curly braces are for readability, but have no effect on the compiled IL. The initializer does, however, change the runtime behavior of your code. You are free to turn off this warning in FxCop, however, this is one that I agree with (it took me a long time to agree with it, though...)
Reed Copsey
+2  A: 

It's relies not on every programmer knowing some locally defined "corporate standard" which might change between at any time, but on something formally defined in the Standard. You might as well say "don't using x++ because that relies on the knowledge of every programmer.

James Curran
completely different issues imo, (between () has DateTime a default, and what is it's value? What does x-- do? I think the answer to the latter 100% of the programmers know, the answer to the first - if we're lucky - 50%)
Peter
First of all, it equals "default(DateTime)". Second, under what circumstances would you care what the actual value is?
James Curran
+2  A: 

You have to remember that FxCop rules are only guidelines, they are not unbreakable. It even says so on the page for the description of the rule you mentioned (http://msdn.microsoft.com/en-us/library/ms182274(VS.80).aspx, emphasis mine):

When to Exclude Warnings:

Exclude a warning from this rule if the constructor calls another constructor in the same or base class that initializes the field to a non-default value. It is also safe to exclude a warning from this rule, or disable the rule entirely, if performance and code maintenance are not priorities.

Now, the rule isn't entirely incorrect, but like it says, this is not a priority for you, so just disable the rule.

casperOne
+3  A: 

FX Cop sees it as adding unnecessary code and unnecessary code is bad. I agree with you, I like to see what it's set to, I think it makes it easier to read.

A similar problem we encounter is, for documentation reasons we may create an empty constructor like this

/// <summary>
/// a test class
/// </summary>
/// <remarks>Documented on 4/8/2009  by richard</remarks>
public class TestClass
{
    /// <summary>
    /// Initializes a new instance of the <see cref="TestClass"/> class.
    /// </summary>
    /// <remarks>Documented on 4/8/2009  by Bob</remarks>
    public TestClass()
    {
        //empty constructor
    }        
}

The compiler creates this constructor automatically, so FX cop complains but our sandcastle documentation rules require all public methods to be documented, so we just told fx cop not to complain about it.

Bob The Janitor
unnecessary code is bad of course, but this code can become necessary in the unlikely event a default will be changed in the future and this code is recompiled.
Peter
@Peter: Defaults for value types are guaranteed to always initialize to zeros. It's part of the CLR specification itself. There is no way for a value type default to change. Reference types are the same - they're guaranteed, by spec, to default to null.
Reed Copsey
@Bob The Janitor, doing things that don't feel right just to satisfy a tool usually ends poorly! ;)
Michael Meadows
it's not doing something to satisfy a tool, I deliberately set the rule that enforces it. If you look at the XML doc the Remarks state when something was Documents, this should be the same time it was created, this then places when something was created into our documentation.
Bob The Janitor
+2  A: 

FxCop has to provide rules for everyone, so if this rule doesn't appeal to you, just exclude it.

Now, I would suggest that if you want to explicitly declare a default value, use a constant (or static readonly variable) to do it. It will be even clearer than initializing with a value, and FXCop won't complain.

private const int DEFAULT_AGE = 0;

private int age = 0; // FXCop complains
private int age = DEFAULT_AGE; // FXCop is happy

private static readonly DateTime DEFAULT_BIRTH_DATE = default(DateTime);

private DateTime birthDate = default(DateTime); // FXCop doesn't complain, but it isn't as readable as below
private DateTime birthDate = DEFAULT_BIRTH_DATE; // Everyone's happy
Michael Meadows
tx (was also suggested in a former comment)
Peter