Code Analysis Prevention CA2000 Warning Using Chain Constructors?

Question

A normal pattern to prevent CA2000 from warning about undefined locales is to use a temporary variable that will be deleted if something goes wrong, like:

Foo f = null; try { f = new Foo(); Foo result = f; f = null; return result; } finally { if (f != null) { f.Dispose(); } } 

A bit verbose, but it works, and that makes sense. But how to apply the template to chain constructors, for example:

 public HomeController ( IDataRepository db ) { this.repo = db ?? new SqlDataRepository(); } public HomeController ( ) : this(new SqlDataRepository()) { } 

This code raises two CA2000 warnings, one for each constructor. The first thing I can get rid of is using the temp variable template. This is quite annoying since there is no way for a local user to get out of scope after creating it, but before it gets assigned to the member field, which will be cleared later. Therefore, I do not know what the CA problem is, but at least I know what to do to fix it.

But, as far as I can tell, there is no alternative way to write a second constructor call to introduce try / finally. The assigned field is read-only, so it should be set in the constructor. And C # will not allow you to call the chain of constructors anywhere, but directly in front of the body of the constructor. And, with regard to the second, the second warning is actually more legitimate - a call to the chain constructor can (theoretically, if it performed any actual work) cause an exception and leave the newly created parameter unchanged.

Of course, I can always just suppress this message (which CA2000 seems to need a lot), but if there is a real way to fix the problem, I would rather do it.

Answer 1

I cannot reproduce the CA2000 violation on a constructor that takes an IDataRepository argument. Given this, along with the fact that you are using the same “default” for both constructors, the simplest change is to avoid problems with the CA2000 for the fetch script:

 public HomeController(IDataRepository db) { this.repo = db ?? new SqlDataRepository(); } public HomeController() : this(null) { } 

Obviously, this will not work if your first constructor did not accept the value of the null parameter. If that were the case, and you are absolutely married with the idea of ​​setting the appropriate field in only one place, you still have options to avoid CA2000, so you are invoking a smarter private constructor. eg:.

 public HomeController(IDataRepository db) : this(() => db, false) { if (db == null) { throw new ArgumentNullException("db"); } } public HomeController() : this(() => new SqlDataRepository(), true) { } private HomeController(Func<IDataRepository> repositoryRetriever, bool disposeOnFailure) { IDataRepository repository = repositoryRetriever.Invoke(); try { this.repo = repository; } catch { if (disposeOnFailure) { repository.Dispose(); } throw; } } 

Personally, I think this is a rather unpleasant hack, especially considering that this is due to the increased complexity of the code and the chance of exceptions at run time in the interests of preventing a potential problem that was not very serious in the first place. My recommendation would be to simply ignore potential CA2000 violations of this kind if both of them are not true:

• There is a real possibility of a no-error exception between the instance of the object and the end of the method in which the field is assigned to it.
• The consequences of refusing to post a lost instance are quite serious (for example: leave the file locked).