Class design: Access a List<T> directly or through methods?

Question

For a newsroom system I have a class that contains a single news story. Inside this class is a private variable holding a generic List of image classes. The idea being a single story can contain multiple images.

The question is should I make the List variable public, so that I can add/remove images by addressing the List directly

public class News
{
    private _images List<Images>();

    public Images
    {
      get { return _images; }
      set { _images = value }
    }
}

or

Should I make the List variable private and then create methods to manipulate it:

public class News
{
    private _images List<Images>();

    public void AddImage( Image image )
    public Image GetImage( int imageId )
    public int GetImageCount()
    public void DeleteImage( int imageId )
}

My spider sense is telling me to do the later, as it's abstracting things more. But on the flip side its creating more code.

Answer 1

By exposing the List, you expose an implementation detail. It will make it easier in the short run, but you'll have a hard time later if you decide to e.g. change the list to some other container (perhaps you need a Dictionary for lookup or something).

I would encapsulate it as it will make the type easier to maintain and enhance in the future.

Answer 2

It depends on whether you need/(would need) to control adding/getting and deleting images or possibility to change container.

Answer 3

If you expose the list as a property then it will be possible to do the following from outside the class:

News.Images = new List<Images>();

Is that what you want? (and you shouldn't because it breaks allsorts of encapsulation principles)

If not then use an ICollection<T> interface:

class News
{
    public ICollection<Image> Images   
    {
        get;
        private set;
    }
 }

or

class News
{
    private List<Image> images = new List<Image>();
    public ICollection<Image> Images   
    {
        get
        {
             // You can return an ICollection interface directly from a List
             return images;
        }
    }
 }

ICollection<T> has methods such as Add, Remove, Clear, Count.

If you want a read-only container return a ReadOnlyCollection

class News
{
    private List<Image> images = new List<Image>();
    public ReadOnlyCollection<Image> Images   
    {
        get
        {
             // This wraps the list in a ReadOnlyCollection object so it doesn't actually copy the contents of the list just a reference to it
             return images.AsReadOnly();
        }
    }
 }

Answer 4

Expose a read only implementation of the list and expose methods for manipulating the list. I would do it like this:

public class News
{
    private IList<Image> _images;

    public void News()
    {
        _images = new List<Image>();
    }

    public void AddImage(Image image) { ... }
    public void RemoveImage(Image image) { ... }

    public IEnumberable<Image> Images
    {
        get { return _images; }
    }
}

Note that the Images property can be cast as List but you can return it in a ReadyOnlyCollection wrapper if needed. Count() and ElementAt() extension methods replace your GetImageCount and GetImage methods.

Answer 5

For accessing the elements you could consider using a ReadOnlyCollection or IEnumerable type. However in order to keep encapsulation ensured, you should use insert/remove methods, this way you don't need the set of the property anymore.

Edit: Someone beat me to it during typing this answer ;)

Answer 6

I think this is a design consideration that only you can decide. Your second approach is hiding an implementation detail that is you used a List to store images. On the other side, the first solution gives you one advantage. You can use all the List methods including those extensions that are always useful. Using the second solution, you can also implement a ToList() method that would return a new constructed List. Changes to this List wouldn't affect the internal structure of your class. The down side is, if the internal Image List is too big, it could affect performance, as it would always build a new List on ToList() but I wouldn't expect this method be called many times.

Another solution would be to expose a ReadOnlyCollection.

Answer 7

If you expose the List directly, you would have to rely on it's mechanics for all Image-related actions (Add, Delete, Count, ...)

I would still expose a collection of Images (ReadOnlyCollection is usually fine) to make the accessing operations of the list easier for both the developer and the consumer, but all create/update/delete logic should be wrapped inside your class.

Answer 8

I would explose a read-only view of the list as IList or IEnumerable, and methods for adding and removing elements. Like this:

public class News
{
    private _images List<Images>();

    public IList<Image> Images
    {
        get {return _images.AsReadOnly(); }
    }

    public void AddImage(Image image)
    {
        _images.Add(image);
        // Do other stuff...
    }

    public void DeleteImage(Image image)
    {
        _images.Remove(image);
        // Do other stuff...
    }
}

Answer 9

Davy Brion made a post on this last week. He favors exposing an IEnumerable<> property and providing an add & remove method for manipulation.

Most of the time you only want to loop through the collection, so IEnumerable<> will do the trick. Besides, you can switch the actual implementation (List, Set, ...) when needed without any hassle, when switching to another ORM this could prove very valuable.

http://davybrion.com/blog/2009/10/stop-exposing-collections-already/

Answer 10

I would just expose the list as an IList:

public class News
{
    private List<Image> _images;

    public IList<Image> Images
    {
        get { return _images; }
        set { _images = value; }
    }
}

If you later want to change the implementation, you can do this without breaking the contract:

public class News
{
    public News(SomeCollection<Image> images)
    {
        _images = images;
        Images = new ListView(this);
    }

    private SomeCollection<Image> _images;

    public IList<Image> Images { get; private set; }

    private class News.ListView : IList<Image>
    {
        public ListView(News news)
        {
            _news = news;
        }

        private News _news;

        // Implement the methods to manipulate _news._images
    }
}