3

I'm not sure if this is going to boil down to a matter of opinion, but I keep seeing a pattern in code that I don't understand.

All over C# apps, I keep seeing the expression: 

System.Configuration.ConfigurationManager.AppSettings["key"].ToString()

For a long time I thought it was just a quirk where I work, but I searched it, and it appears it's not just a few one-off things. People do it a fair amount, but it isn't really talked about as far as I can see.

This blog, for example, makes sure to call .ToString() on AppSettings, and so does this blog

Moreover, all the programmers on this question make sure to call .ToString() on the AppSettings collection.

To make sure I'm not going crazy, I rolled over the method in Visual Studio, which assures me that, yes, AppSettings[key] is strongly typed as a string and that "no actual conversion is performed" when calling .ToString().

It would seem to me the only real difference between AppSettings[key] and AppSettings[key].ToString() is the possibility of throwing a NullReferenceException.

So my question is:

Is there any technical reason for doing this, or is it a widely-recommended C# best practice for some reason, or is it just a weird quirk with no real meaning?

Eleanor Holley
  • 691
  • 1
  • 7
  • 27
  • 1
    You'll see this pattern a lot in bad code. In general if I see `.ToString()` anywhere except perhaps a StringBuilder, I'm suspicious. It's a code smell. I've updated those answers you linked to so that they no longer call .ToString(). If you see it anywhere else, feel free to leave an answer calling them out on it! – mason Jan 29 '18 at 15:04
  • given that `AppSettings["key"]` returns a string, the following `ToString()` it's at best not useful at all, and at worst can cause an exception if `AppSettings["key"]` returns `null` – Gian Paolo Jan 29 '18 at 15:06
  • @mason Will do, thanks! – Eleanor Holley Jan 29 '18 at 15:23
  • @EleanorHolley I originally said "leave an answer calling them out". The should have said "a comment" instead of "an answer". You should only leave an answer if you directly address the question being asked. Just wanted to make sure I didn't steer you in the wrong direction. I swear, it all sounds right in my head! – mason Jan 29 '18 at 15:25

2 Answers2

6

Is there any technical reason for doing this, or is it a widely-recommended C# best practice for some reason, or is it just a weird quirk with no real meaning?

The latter. There is no use in calling ToString. It can only do harm.

ConfigurationManager.AppSettings is of type NameValueCollection, from which the accessor returns a string already. Calling ToString can only cause a NullReferenceException. It has no positive effects.

Patrick Hofman
  • 153,850
  • 22
  • 249
  • 325
5

I would suggest a strong reason why such code likely exists is it was probably originally written1 before ConfigurationManager was introduced in .NET 2.

Previously, one would access the Configuration class with its AppSettings property that returned an AppSettingsSection. Which yes, unfortunately, has an indexer that returns object rather then string.

In upgrading their code to .NET 2, they'll have seen a suggestion to use ConfigurationManager instead and will have performed the minimal amount of editing to make their code use this class instead. And not thought about/considered removing the trailing ToString() that is now redundant.

1As discussed in the comments, it was probably originally written long ago and upgraded long ago. But this is the sort of code that will be copy & pasted a lot and so perpetuate itself almost virally.

Damien_The_Unbeliever
  • 234,701
  • 27
  • 340
  • 448