The best way to throw an exception

Question

Do you know a better way (more pretty) than below to throw an exception?

public long GetPlaylistId(long songInPlaylistId)
{
    var songInPlaylist = service.GetById(songInPlaylistId);
    return songInPlaylist
            .With(x => x.Playlist)
            .ReturnValueOrException(x => x.Id, 
                                         new ArgumentException(
                                             "Bad argument 'songInPlaylistId'"));
}

Monadic extension methods:

public static TResult With<TInput, TResult>(this TInput obj, 
                                            Func<TInput, TResult> evaluator)
    where TInput : class
    where TResult : class
{
    return obj == null ? null : evaluator(obj);
}

public static TResult ReturnValueOrException<TInput, TResult>(
    this TInput obj, Func<TInput, TResult> evaluator, Exception exception)
    where TInput : class
{
    if (obj != null)
    {
        return evaluator(obj);
    }

    throw exception;
}

Do not use exceptions to control the flow of a program. You should raise an exception when there is no other options to solve the problem. http://msdn.microsoft.com/en-us/library/dd264997.aspx — Tim Schmelter, Apr 10 '14 at 07:23
@Tim Schmelter, I think that in such case I have to throw an exception. What will I may return for your opinion? — Neshta, Apr 10 '14 at 07:41
@Neshta: one approach: return `Nullable`, another: `TryGetPlaylistId` with `out` parameter and `bool` as return value. — Tim Schmelter, Apr 10 '14 at 07:50
It's perfectly acceptable to throw exceptions in cases where you expect the parameter to be valid. For example, using `new FileStream(filename, FileMode.Open);` will throw a `FileNotFoundException` exception if the file doesn't exist. — Matthew Watson, Apr 10 '14 at 07:55
If the suggested null-propagating operator was in the language, he might (depending on its exact definition) `return songInPlaylist?.Playlist?.Id;`. — Jeppe Stig Nielsen, Apr 10 '14 at 08:09

Matthew Watson · Answer 1 · 2014-04-10T08:13:12.490

If it is valid to try to get the playlist for something that doesn't have a playlist, then you should not throw an exception but should just return a special value that means "not found" instead (for example, 0 or -1 depending on how your playlist IDs work).

Alternatively you could write a TryGetPlaylistId() method which works in a similar way to Microsoft's TryXXX() methods (e.g. SortedList.TryGetValue()), for example:

public bool TryGetPlaylistId(long songInPlaylistId, out long result)
{
    result = 0;
    var songInPlaylist = service.GetById(songInPlaylistId);

    if (songInPlaylist == null)
        return false;

    if (songInPlaylist.Playlist == null)
        return false;

    result = songInPlaylist.Playlist.Id;
    return true;
}

A small problem with this approach is that you are obscuring information that might be of use when trying to diagnose issues. Perhaps adding Debug.WriteLine() or some other form of logging would be of use. The point being, you can't differentiate between the case where the playlist ID is not found, and the case where it is found but doesn't contain a playlist.

Otherwise, you could throw an exception which has a more informative message, for example:

public long GetPlaylistId(long songInPlaylistId)
{
    var songInPlaylist = service.GetById(songInPlaylistId);

    if (songInPlaylist == null)
        throw new InvalidOperationException("songInPlaylistId not found: " + songInPlaylistId);

    if (songInPlaylist.Playlist == null)
        throw new InvalidOperationException("Playlist for ID " + songInPlaylistId " has no playlist: ");

    return songInPlaylist.Playlist.Id;
}

It might be the case that it is valid to not find the song in the playlist, but it is NOT valid to find one which does not have a playlist, in which case you would return a special value in the first case and throw an exception in the second case, for example:

public long GetPlaylistId(long songInPlaylistId)
{
    var songInPlaylist = service.GetById(songInPlaylistId);

    if (songInPlaylist == null)
        return -1; // -1 means "playlist not found".

    if (songInPlaylist.Playlist == null)
        throw new InvalidOperationException("Playlist for ID " + songInPlaylistId " has no playlist: ");

    return songInPlaylist.Playlist.Id;
}

In any case, I personally think that your extension methods are just obscuring the code.

score 0 · Answer 2 · answered Apr 10 '14 at 08:01

0

try{
if (obj != null)
{
    return evaluator(obj);
}
}
catch(Exception ex)
{
throw;
}

return obj;

You should not throw error unless caught in to some. Better return null in the given case and handle it in your calling code:

answered Apr 10 '14 at 08:01

Soumitra Bhatt

59
6

if code encounter any error then only it is going to throw error otherwise return with the obj(whatever value it is holding) – Soumitra Bhatt Apr 10 '14 at 09:49
In your case the try/catch doesn't do anything at all. The code is functionally equivalent, if you remove it – thumbmunkeys Apr 10 '14 at 10:02
How is it similar!this code will only return null in case of obj==null whereas asked code will throw exception in case of null!! – Soumitra Bhatt Apr 10 '14 at 10:25
please stop it with the exclamation marks, it is considered impolite. Try it in a program and see for yourself. Also see this answer: http://stackoverflow.com/a/881489/451540 – thumbmunkeys Apr 10 '14 at 10:48
Sorry if feel impolite :) i'll remove '!' – Soumitra Bhatt Apr 10 '14 at 10:58
@thumbmunkeys i read it , The article is not saying anything much different. But thanks for sharing. – Soumitra Bhatt Apr 10 '14 at 11:06
it says: `Second, if you just catch and re-throw like that, I see no added value, the code example above would be just as good (or, given the throw ex bit, even better) without the try-catch.` – thumbmunkeys Apr 10 '14 at 11:12
"Throw ex" will change the stack trace. And see in the asked code the guy is managing flow of program using 'throw' that is why i said it is not good to use throw like this. It is better to throw if encountered any exception, according to the need and architecture of application. – Soumitra Bhatt Apr 10 '14 at 11:20
as I said: try your code with and without the try/catch. You will see no functional difference – thumbmunkeys Apr 10 '14 at 11:37

score 0 · Answer 3 · answered Apr 10 '14 at 09:11

And what will happen if I have more than one such ambiguous methods in my class? It's very difficult to invent different rules for any method. You will be confused in the end. What do you think about this solution?

public class ApplicationResponse
{
    public IList<string> Errors { get; set; }
    public dynamic Data { get; set; }

    public bool HasErrors()
    {
        return Errors != null && Errors.Any();
    }
}

public ApplicationResponse GetPlaylistId(long songInPlaylistId)
{
    var songInPlaylist = service.GetById(songInPlaylistId);
    if (songInPlaylist == null)
    {
        return new ApplicationResponse { Errors = new[] { "Song was not found." } };
    }

    if (songInPlaylist.Playlist == null)
    {
        return new ApplicationResponse { Errors = new[] { "Playlist was not found." } };
    }

    return new ApplicationResponse { Data = songInPlaylist.Playlist.Id };
}

public HttpResponseMessage SomeRequest([FromUri] songInPlaylistId)
{
    var response = appService.GetPlaylistId(long songInPlaylistId);
    if (response.HasErrors())
    {
        // reply with error status
    }

    // reply with ok status
}

In such case I can send all the errors to a client.

The best way to throw an exception

3 Answers3