So, I ran my project through Gendarme. The results were... distressing, to say the least. It got me thinking about how to improve the readability of my codebase, and actually adhere to the more idiomatic rules of C#. I've fixed several of the most obvious issues, but two things in particular has been bothering me. The first is the AvoidVisibleConstantFieldRule. The rationalization for it I understand, but this presents a problem given this (common, I'd say) structure:



public MyClass(int type, params double[] vals)
{
static const int MIN = 1;
static const int MAX = 2;
static const int AVG = 3;
switch(type)
{
case MIN:
//etc...
}
}


They recommend "static readonly", but that doesn't work in the switch. Is the above code Not Right™ in C#? Obviously, I could convert everything to if-else, but that seems like it's dodging the issue.

The other issue is the idea that a parameter should not have the same name as a field (VariableNamesShouldNotMatchFieldNamesRule). This one pops up all over the place. I don't understand this one at all. It's common for certain functions (i.e. constructors) to do a lot of variable setting. The constructor parameters need to specify exactly what data you're sending, and if I had a better name than the field name for the data in question, that would be the name of the field in the first place! For example:


public class MyObject
{
Vector position;
int health;
int speed;
public MyObject(Vector position, int health, int speed)
{
this.position = position;
this.health = health;
this.speed = speed;
}
}


How on earth could this be suboptimal as a naming convention?

For your first question, it seems to me that you should be using an enum for that?

They recommend "static readonly", but that doesn't work in the switch. Is the above code Not Right™ in C#? Obviously, I could convert everything to if-else, but that seems like it's dodging the issue.[/quote]

Ignoring various clutter that arises from applying patterns blindly, code that uses switch to distinguish behavior is replaced with strategy pattern in C#/Java. It ends up looking something like this:class MyClass {
Policy policy;
void apply() {
policy.apply(myData);
}
}

interface Policy {
void apply(T myData);
}

class MinPolicy : Policy {
void apply(...) { }
}
class MaxPolicy : Policy {
void apply(...) { }
}

The constructor parameters need to specify exactly what data you're sending, and if I had a better name than the field name for the data in question, that would be the name of the field in the first place![/quote]

An age old problem, first made prominent by Delphi, which chose to prefix parameters passed into constructor with 'a', so aPosition, aVelocity.

At design level it's an indication that you're adding needless indirection. Consider instead this:class MyObject {
public MyObject(float x, float y, float z); // create from position
public MyObject(Defaults d) // create from config file
{
this.position = new Vector();
this.position.x = d.get("initial_x", 0.0f); // get x or set to 0.0 if missing
...
}
}

Reason behind such approach is encapsulation and separation of concerns. By passing in velocity and all other values that are identical in class structure, you effectively expose internal details. Ideally, users of class should know nothing of Vector type nor internal representation. If such 1:1 pairing appears, it's an indication of class being effectively a POD (plain old data type). Then one might as well expose all internal members directly and set them as such.

That said, it's a common issue with C# and Java to which there is no clean solution. Don't worry about it, either adopt the prefix naming of parameters or turn off the warning.

There's a reason for this warning and as far as big picture goes it's a useful one, but likely not worth solving at this point. Many APIs however could be vastly improved (aka reduced in size by orders of magnitude) if they understood the idea behind this warning.

There's a reason for this warning and as far as big picture goes it's a useful one, but likely not worth solving at this point. Many APIs however could be vastly improved (aka reduced in size by orders of magnitude) if they understood the idea behind this warning.

Do you have link(s) to a detailed explanation? I've used architectures that try to encapsulate and decouple things to the point where the library is no longer usable.

The main reason I see constructor-initializing-fields in C# is to allow you to create immutable types.

I typically use this pattern:

public T Field { get; private set; }
ctor(T field)
{
Field = field;
}

Thanks for the suggestions, guys. As far as the const/switch stuff goes, I'll probably just use the strategy pattern as Antheus suggested. One of my classes was really ugly and hacked together because of this, and I think the end result looks a lot cleaner. It makes adding new cases easier, too (although I think I've been working on this project long enough that I've covered all of them).

As far as the variable naming issues go, I'll probably just ignore them. I don't like going against Best Practices(TM), but there are a lot of situations where there's no sensible way to do things. I'm working on a noise library, and here half the classes take siblings as (perhaps their only) parameters for constructors. The conflict is nonexistent past the constructor and the code won't compile unless I prefix the fields to be assigned with the this keyword anyway, so I don't see any way it could become a headache-inducing decision in the future.