using indexer to retrieve Linq to SQL object from datastore

Question

class UserDatastore : IUserDatastore
{
    ...

    public IUser this[Guid userId]
    {
        get
        {
            User user = (from u in _dataContext.Users
                             where u.Id == userId
                             select u).FirstOrDefault();

            return user;
        }
    }

    ...
}

One of the developers in our team is arguing that an indexer in the above situation is not appropriate and that a GetUser(Guid id) method should be prefered.

The arguments being that:

1) We aren't indexing into an in-memory collection, the indexer is basically performing a hidden SQL query 2) Using a Guid in an indexer is bad (FxCop flagged this also) 3) Returning null from an indexer isn't normal behaviour 4) An API user generally wouldn't expect any of this behaviour

I agree to an extent with (most of) these points.

But I'm also inclined to argue that one of the characteristics of Linq is to abstract the database access to make it appear that you're simply working with a bunch of collections, even though the lazy evaluation paradigm means those collections aren't evaluated until you run a query over them. It doesn't seem inconsistent to me to access the datastore in the same manner as if it was a concrete in-memory collection here.

Also bearing in mind this is an inherited codebase which uses this pattern extensively and consistently, is it worth the refactoring? I accept that it might have been better to use a Get method from the start, but I'm not yet convinced that it's completely incorrect to be using an indexer.

I'd be interested to hear all opinions, thanks.

Answer 1

I think his point regarding using a GUID as an indexer is probably referring to it being stored in the Database. Using an integer as the key would provide better performance and would take up less storage than using GUIDs.

Returning null is quite uncommon as usually with an indexer if the index (key) is not in the collection you would throw an exception instead.

Personally I don't really see the issue here it is pretty much the equivalent of having a GetUser method anyway. I mean if you wanted to tidy it up a bit you could actually introduce a private method called GetUser that the indexer could call e.g.

public IUser this[Guid userId]
{
    get { return GetUser(userId); }
}

private IUser GetUser(Guid userId)
{
    return (from u in _dataContext.Users 
            where u.Id == userId 
            select u).FirstOrDefault();
}

Looking from a design point of view, personally I would not have used an Indexer, I would have went with a GetUser method.

Answer 2

I tend to agree with almost everything your team developer suggested, except the Guid part. If there's any problem here, it must be at the database level considering performance, not in your code.

Personally I think Properties and Indexers should not hide long running operations, although the Users could be cached by LINQ-to-SQL. That's why I would also prefer a method.

I also agree that returning a null from an indexer is bad. Throw an exception instead.