Correct Use of .NET Exception

Question

What is the correct exception to throw in the following instance?

If, for example, I have a class: Album with a collection of Songs:

List<Song>

And a method within Album to add a Song:

public void AddSong(Song song)
{
    songs.Add(song);
}

Should I throw an exception if a user attempts to add a song that already exists? If so, what type of exception?

I have heard the phrase: "Only use exceptions in exceptional circumstances", but I want to tell the client implementing Album exactly what has gone wrong (not just return a Boolean value).

Answer 1

You can always create your own exceptions. Simply create a class that inherits from Exception(or, in this case, ArgumentException).

Something along the lines of DuplicateItemException (or DuplicateSongException if you want something very specific) sounds about right.

Answer 2

If your use case implies that items in the collection should be unique, then you should use a datastructure that enforces that.

By doing that, you not only avoid having to write a O(N) lookup method to check for duplicates, but you can also bubble up the pre-existing duplicate key exception that a collection of this sort would throw.

However, .NET does not have a distinct collection that preserves sort order, though it is very easy to extend List to support this.

The approach I used below sacrifices memory footprint for speed, by storing the unique values in a second HashSet. If memory size was more important, you'd just have to do a O(N) check on each Add operation. Because methods are not virtual (for some reason) in List, I resulted to hiding the base methods using the new keyword.

Note that this is just an example, and is not thread safe, and should probably not be used in a real production application.

    public class UniqueList<T> : List<T>
    {
        private HashSet<T> _internalHash = new HashSet<T>();

        public UniqueList() : base() { }
        public UniqueList(IEnumerable<T> collection) : base(collection) { }
        public UniqueList(int capacity) : base(capacity) { }

        public new void Add(T item)
        {
            if (!_internalHash.Add(item))
                throw new ArgumentException("Item already exists in UniqueList");

            base.Add(item);
        }

        public new void AddRange(IEnumerable<T> collection)
        {
            foreach (T t in collection)
            {
               this.Add(t);
            }
        }

        public new bool Remove(T item)
        {
            _internalHash.Remove(item);
            return base.Remove(item);               
        }

        public new int RemoveAll(Predicate<T> match)
        {
            int removedElems = 0;

            foreach (T item in this)
            {
                if (match(item))
                {
                    this.Remove(item);
                    removedElems++;
                }
            }

            return removedElems;
        }

        public new void RemoveAt(int index)
        {                
           this.Remove(this[index]);             
        }

        public new void RemoveRange(int index, int count)
        {
            for (int i = index; i < count; i++)
            {
                this.Remove(this[i]);
            }
        }
    }

Answer 3

Instead of throwing an exception you could have the AddSong method return a boolean - true if the song is successfully added and false otherwise. Personally, I think throwing an exception would be acceptable in this case if it's reasonable to expect that the song is unique in the collection. For an example, if the collection is a list of songs on an album you don't reasonable expect a duplicate song (same title, same duration, same position in the sequence of tracks, etc.). You have the option of creating your own exception class derived from System.Exception to create custom errors if you want so you could throw an exception that explains exactly why the error occurred.

Answer 4

If you want to offer up useful exceptions you may want to have a base exception.

AlbumException

Then taking from CMerat's answer create.

DuplicateSongException

This should of course inherit from AlbumException.

Personally, I would make the Album class immutable. In that case this whole situation would disappear.

Answer 5

In exactly the same situation, the .NET designers at Microsoft chose to throw ArgumentException with a descriptive message. Oh, and they were pretty consistent about that.

Answer 6

If adding duplicate songs is not an expected use scenario for your API, you should throw an Exception, yes.

However by looking at the given example, since you're implementing an album, "duplicate song" seems to be a problem that should be avoided by API design.

First, I think you should have a method to check if the album contains a certain song is in the library; ContainsSong for instance. The client should know that duplicate songs are not expected and they need to be provided with necessary tools to avoid that by themselves.

This way client can check for duplicates before the actual call. In fact, a client in this case may not even allow "Add"ing in the first place, by disabling the "Add" button or not showing it at all. (Or providing nice hint messages). The point is, if this is something you don't expect, your design should be shaped in a way to avoid it.

Then after having this kind of nice layout, you can still throw your exception, because in this case, it would have really been "unexpected" to get a duplicate song from the client.

Otherwise having just the Exception narrows down client's options to handle the situation smoothly. It eventually may turn out to be a problem of "call Open inside a try block to see if file exists", which promotes drug use.