views:

341

answers:

5

Suppose you have a class Person :

public class Person
{
   public string Name { get; set;}
   public IEnumerable<Role> Roles {get; set;}
}

I should obviously instanciate the Roles in the constructor. Now, I used to do it with a List like this :

public Person()
{
   Roles = new List<Role>();
}

But I discovered this static method in the System.Linq namespace

IEnumerable<T> Enumerable<T>.Empty<T>();

From MSDN:

The Empty(TResult)() method caches an empty sequence of type TResult. When the object it returns is enumerated, it yields no elements.

In some cases, this method is useful for passing an empty sequence to a user-defined method that takes an IEnumerable(T). It can also be used to generate a neutral element for methods such as Union. See the Example section for an example of this use of

So is it better to write the constructor like that? Do you use it? Why? or if not, Why not?

public Person()
{
   Roles = Enumerable.Empty<Role>();
}
+4  A: 

I think Enumerable.Empty is better because it is more explicit: your code clearly indicates your intentions. It might also be a bit more efficient, but that's only a secondary advantage.

Tommy Carlier
Yes, making it clear that you mean this to be empty vs. something that might be added to later is good for your code.
Jay Bazuzi
Yes. Enumerable<>.Empty does avoid allocating a new object, which is a small bonus.
Neil Whitaker
+4  A: 

The problem with your approach is that you can't add any items to the collection - I would have a private structure like list and then expose the items as an Enumerable:

public class Person
{
    private IList<Role> _roles;

    public Person()
    {
     this._roles = new List<Role>();
    }

    public string Name { get; set; }

    public void AddRole(Role role)
    {
     //implementation
    }

    public IEnumerable<Role> Roles
    {
     get { return this._roles.AsEnumerable(); }
    }
}

If you intend some other class to create the list of roles (which I wouldn't recommend) then I wouldn't initialise the enumerable at all in Person.

Lee
that's a good point. I would have find that out quickly in my implementation in fact :)
Stephane
I would make _roles readonly, and I don't see the need for the call to AsEnumerable(). Otherwise, +1
Kent Boogaart
@Kent - I agree with readonly. As for AsEnumerable, it's not really needed but it prevents Roles being cast to IList and modified.
Lee
+5  A: 

I think it's totally fine and readable/maintainable to be old-skool:

Assuming you really want to populate the Roles property somehow:

public class Person
{
    public string Name { get; set; }
    public IList<Role> Roles { get; private set; }

    public Person()
    {
        Roles = new List<Role>();
    }
}
Neil Barnwell
In the end, Keep it simple stupid wins.. Thank you
Stephane
+1  A: 

The typical problem with exposing the private List as an IEnumerable is that the client of your class can mess with it by casting. This code would work:

  var p = new Person();
  List<Role> roles = p.Roles as List<Role>;
  roles.Add(Role.Admin);

You can avoid this by implementing an iterator:

public IEnumerable<Role> Roles {
  get {
    foreach (var role in mRoles)
      yield return role;
  }
}
Hans Passant
If a developer casts your IEnumerable<T> to a List<T>, then that's his problem and not yours. If you change the internal implementation in the future (so it's no longer List<T>), it's not your fault that the developer's code breaks. You can't stop people from writing crappy code and abusing your API, so I don't think it's worth spending a lot of effort on.
Tommy Carlier
This is not about breaking code, it is about exploiting code.
Hans Passant
A: 

I am not a fan of using List everywhere. I would rather use an array:

public class Person
{
   public string Name { get; set;}
   public IEnumerable<Role> Roles {get; set;}

   public Person
   {
      this.Roles = new Role[0];
   }
}
Jader Dias