Jump to content
  • Advertisement
Sign in to follow this  
SeraphLance

Keeping friendly with static analysis

This topic is 2411 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

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?

Share this post


Link to post
Share on other sites
Advertisement

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

if (position == 4)
Which position is being referred to here? The specific problem is that the initial "this." is not mandatory; I think it should be (and it was also an opportunity missed in C++ standards) because it makes everything nice and unambiguous (and also makes the argument for using ugly-looking g_ and m_ prefixes go away), but it's not.

I always camel-case members and lower-case params.

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites

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;
}

Share this post


Link to post
Share on other sites
I'm a fan of the syntax where you prefix member variables with an underscore ie :

_position = position

or use an Auto Property:

Position = position

I use Resharper for my code analysis, and it recommends either of those two depending on the context.

I like to write ctor's with passed in parameters whenever the field being initialised is a "must initialise" field, ie there's no sane situation where that field could ever not be set, and there is no suitable default, like for example a reference to an essential object.

The original code you pasted with the statics and the case statement doesn't feel right at all for c#, it seems like something I would have coded back in the original C days. I'd recommend using an enumerated type as suggested by DvDmanDT.

Something like:

enum MathsOperation { Minimum, Maximum, Average }
...
switch (operation)
{
case MathsOperation.Minimum:
...
case MathsOperation.Maximum:
...
case MathsOperation.Average:
}

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!