Exposing a null Object Instance as a Property

Question

Today I was looking at some code atually from Nikhil Kothari's Facebook.NET API. I just wanted to check out how he was doing some things in there for ideas.

One of the things I came across was a Property that just seemed really weird to me.

check this out:

FacebookRequest.cs defines a property and sets it in the constructor to a new instance of a custom Class:

    public FacebookRequest(FacebookSession session) {
        ....
        _parameters = new FacebookRequestParameterList();
    }

private field:

private FacebookRequestParameterList _parameters;

and the property:

    public FacebookRequestParameterList Parameters {
        get {
            return _parameters;
        }
    }

now the FacebookRequestParameterList is actually a Generic Dictionary because it inherits & extends Dictionary:

public sealed class FacebookRequestParameterList : Dictionary<string, string> {
...
}

Ok so essentially when you instantiate FacebookRequest, it therefore automatically comes with an auto-instantiated instance of the FacebookRequestParameterList class. So the Property is essentially returning an instance of FacebookRequestParameterList.

Is that normal? I don't think I've seen that a lot. Seems sneaky to me or is this standard stuff here? It just seemed like an odd way to do it. I'm not posting this to bring him down but to understand if this is something standard or sneaky/evil.

It seems to me it would be better to require developers to pass in an instance of FacebookRequestParameterList through the constructor of FacebookRequest instead. And then work with that by setting the private field _parameters after you initialize it through the constructor. Why do I think this is better? Because then developers know exactly what's going on. They know that the class expects a parameter list up front. And just exposing an instance like that through a properly to me just seems odd.

Am I off base here?

Answer 1

I don't think it's odd at all. It saves the client the trouble of having to instantiate the FacebookRequestParameterList instance, and the FacebookRequest class is guaranteed that _parameters contains a valid instance of the FacebookRequestParameterList class, and isn't null.

It's a matter of convenience for the client, and object validity for the class itself.

Nothing to see here, people, move along.

(Edit: clarified the specific instance in the first paragraph)

Answer 2

The usage is perfectly valid. The class simply uses a Dictionary<string,string> internally and exposes that as a property. There's presumably no need to pass in a pre-baked dictionary to the constructor.

What may be better would be to expose it as IDictionary<string,string> instead of an instance of a concrete class.

Answer 3

Have you ever used a DataTable? When you instantiate a new DataTable, you automatically have a collection of rows and columns. Would it make sense to have to instantiate a DataTable and then pass it in your own collection of rows and columns? Obviously not. They're already there for you, and you can add stuff to them as you see fit.

(I just picked a DataTable because I think that makes it easy to understand. I'm sure there are many other examples out there.)

Answer 4

I it is normal, and not sneaky at all. Since the collection property that is exposed is read only (you can't replace the whole collection at once), it assures you that you have an instance to work with (unless the owning class does something funky with it).

I do agree though that it might be a better design, but I don't know about the rest of the API, so I can't comment if it is good or bad in that context. Generally though, I'd agree, the parameters should be passed in (and copied to another dictionary quite frankly, since the session object seems to have a lifetime of some sort).

Answer 5

I don't think there is anything wrong with it. You could then do stuff like fbreq.Parameters["param"]="value" in a very intuitive way, while requiring the dictionary to be provided on construction can be more confusing (or distracting) and you may not even now all the parameters before the request is created.