Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

StyleCop Madness: protected field

I was commisioned to make other people's code compliant with some StyleCop ruleset, as far as possible.. now I was wondering about the following case:

I have an abstract class that contains a field

protected double[] coefficients;

Stylecop says: SA1401: Fields must be declared with private access. Use properties to expose fields.

So I changed it to:

protected double[] coefficients { get; set; } 

Stylecop says: SA1300: property names begin with an upper-case letter: coefficients.

Since it's an abstract class, the child classes are beyond my control. And they use their parent's class field base.coefficients.. well.

So.. there is no way other way to get rid of this warning than to suppress (or disable) it, right? :T

like image 887
Efrain Avatar asked May 03 '11 13:05

Efrain


3 Answers

Well, actually you don't.

If you want to get it clean in future releases of the code, you could set the coefficients property deprecated and provide a message that states that this property will be removed in any further version of the code.

In the current release:

private double[] _coefficients;

// Using this property will produce a compile-time warning.
[Obsolete("coefficients will be removed in future versions. Use Coefficients instead.")]
protected double[] coefficients 
{ 
    get { return _coefficients; } 
    set { _coefficients = value; } 
}

protected double[] Coefficients 
{ 
    get { return _coefficients; } 
    set { _coefficients = value; } 
}

In a later release:

private double[] _coefficients;

// Using this property will produce a compile-time error.
[Obsolete("coefficients was removed. Use Coefficients instead.", true)]
protected double[] coefficients 
{ 
    get { return _coefficients; } 
    set { _coefficients = value; } 
}

protected double[] Coefficients 
{ 
    get { return _coefficients; } 
    set { _coefficients = value; } 
}
like image 93
Florian Greinacher Avatar answered Oct 11 '22 04:10

Florian Greinacher


Even changing fields to properties is a potentially breaking change. If you have to retain compatibility with compiled binaries that you have no control over, I don't even recommend making the first change.

The thing you have to remember about StyleCop is that it doggedly insists that you follow coding guidelines, which are not really the same thing as rules. Both of the guidelines you've run into are perfectly reasonable. I always follow them when writing new code. You should, too. However, if, as in your case, compatibility is more important, then it's probably permissible to ignore StyleCop's insistence.

Even if you were to solve the current warning, the next thing it'll probably hit you with (or if it doesn't, it should, and FxCop definitely does catch this) is that arrays should never be exposed as part of an interface (meaning, marked protected or public). Instead, you should expose something like IEnumerable<T>, IList<T> or ReadOnlyCollection<T>.

Rinse, lather, repeat. You're just chasing your tail if you can't really change the code.

And just to see if I can drive you even more insane, I'll note that FxCop would throw a fit if it were to discover that the child classes were explicitly calling a member on their parent class if they don't override the implementation of that member themselves. So basically, base.coefficients is no-no in the first place. If you follow all the guidelines, it makes following them easier. The logical corollary, of course, is that if you don't follow all of the guidelines, it makes following them much more difficult.

like image 29
Cody Gray Avatar answered Oct 11 '22 04:10

Cody Gray


If you change field to property, the derived classes (from other compiled assemblies) will not find "coefficients", because property is shortcut for two methods - not equals with field.

If you must hold compatibility with other assemblies, the compatibility must be primary and StyleCop rules secondary goal.

like image 30
TcKs Avatar answered Oct 11 '22 04:10

TcKs