What is better to return from the DataService: Status or Exceptions?

Question

I have a MVVM app with DataServices (using mvvmlight).

Right now, i'm using it like

var answer = await myDataService.PullList(categoryId);
if (answer.Status == Ok)
    ObservableListOfItems.ReplaceWith(answer.Collection); // Show results
if (answer.Status == ConnectionLost)
    // Handle connection lost state, show msg window or something
if (answer.Status == ParsingError)
    // Handle parsing problem

where

async Task<MyAnswer> PullList(int id)
{
    // Just a wrapper around HttpWebRequest, returns String.Empty if something happened
    var resultString = await NetworkManager.GetAsync(UrlGenerator.GenerateCategoryRequest(id));  

    if (String.IsNullOrEmpty(resultString))
        return new MyAnswer {Status = ConnectionLost);

    try
    {
        var parsedResult = JsonConvert.DeserializeObject<PocoProducts>();

        return new MyAnswer {Status = Ok, Collection = parsedResult.List);
    }
    catch (Exception)
    {
         return new MyAnswer {Status = ParsingError; }
    }

and

public class MyAnswer()
{
    public List<Items> {get; set;}
    public EnumStatuses Statuc {get; set;}
}

public enum EnumStatuses
{
    Ok = 0,
    ConnectionLost,
    ParsingError
}

So I'm curious, did i just re-invented an exceptions, didn't i?

PS: wrote a code just here, typos are possible, but general idea should be clear

I've migrated this question to Programmers because it is more a design review than a Code Review. Distinguishing characteristics include the nature of the question, the presence of stub code (placeholder comments), a "general idea" disclaimer, and an actual typo in the code. — 200_success, May 20 '14 at 10:17

score 3 · Answer 1 · answered May 17 '14 at 20:42

I guess my main issue with this is

What happens if an exception occurs during parsing. How will you diagnose it or know what has occurred?

The reason I ask this is that the exception itself is ignored and you are simply returning a new MyAnswer. I don't necessarily have any issues with this if you are logging the exception however this does not appear to be the case here.

Included in this, you are catching a general Exception? Does DeserializeObject throw specific exceptions? I'm not 100% on that but where possible catch the most specific exception you know about and leave the rest to bubble up. A reason for this is that your code is then easier to follow as you know exactly what you are dealing with in the method and can code accordingly.

So in summary. In general I like to at the very least do something with the exception whether it's re-throwing, including it in the response or logging it. I also like to catch the most specific exception I can at the point of interest to ensure the code is easier to follow and in the long term help provide a more robust solution.

Well, it can throw, but it shouldn't. :) Afaik, wp7 protocol (tcp-ip, if i recall correctly) guarantees full packet delivery - that excludes any data corruption. If server sends any unexpected POCOs (like `PocoError` instead of `PocoProducts`), it should warn about it in a status, which can be checked separately like *if ((int) JObject.Parse(resultString)["status"] != 200) return new MyAnswer {Status = ServerRequestError};* But anyways, any parsing troubles would return a `new MyAnswer` **with Status = ParsingError** so we can handle it in the ViewModel. — , May 17 '14 at 20:51

score 1 · Answer 2 · answered May 19 '14 at 18:49

I have a some apps which use code like your code. I use HttpClient and have a few types errors. For example MyWebException.

It is enough to keep the user informed. For example, if the Web server is not available, we will get response.StatusCode != HttpStatusCode.OK and throw the MyWebException. And if we got json but could not parse it, then throw Exception.

If we get MyWebException then we show message "Sorry, we have troubles with web server or bad signal". If Exception then "Sorry, something wrong".

You can even specify the error, if it is necessary.

Here's how it works:

API:

 public async static Task<CPParkingZones> GetZones()
        {           
            return await CPRequestService.Get<CPParkingZones>(string.Format("objects?types=zone"));        
        }

RequestService:

public async static Task<T> Get<T>(string url) where T : CPBase, new()
        {
            try
            {                
                using (var httpClient = new HttpClient())
                {
                    var response = await httpClient.GetAsync(CPConfig.APIServer + url);
                    var responseObject = new T();
                    if (response.StatusCode != HttpStatusCode.OK)
                    {
                        responseObject.Error = new MyWebException();
                        return responseObject;
                    }
                    response.EnsureSuccessStatusCode();
                    string jsonResponse = await response.Content.ReadAsStringAsync();
                    responseObject = JsonConvert.DeserializeObject<T>(jsonResponse);
                    responseObject.Status = response.StatusCode;
                    return responseObject;
                }
            }
            catch (Exception ex)
            {
                var responseObject = new T();
                responseObject.Error = ex;
                return responseObject;
            }
        }

Base class

public class CPBase 
{                    
        public Exception Error { get; set; }
}

Using

var zones = await CPMapObjectsAPI.GetZones();

if (zones.Error != null)
{   
    if (zones.Error is WebException)
    {  
       DialogService.ShowMessage("Sorry, we have troubles with web server or bad signal");    
    }  
    else
    {
       DialogService.ShowMessage("Sorry, something wrong");    
    }         
}

This is just an example, "IF constructs" design looks awkward, especially If you need many types of errors, you can use many options to write this code better.

Just a small note i got several days ago, sharing to you ( http://codereview.stackexchange.com/a/49853/31091 ): `The Task-based asynchronous pattern says that the names of Task-returning methods should end with Async, e.g. ReadJsonAsync.` :) | But in general, approach seems to be quite similar to mine: actually, you're not throwing Exception from your `Get()` but just saving it. You can rename it to Enum and it would be pretty the same :) — , May 19 '14 at 20:51
yes, you're absolutely right. but in the try catch block you can save different exceptions, not need to create a status for them in the enum. but it's little nuances. Generally I wanted to show that I have same method to handle exceptions and problems have not yet been. :) — , May 19 '14 at 21:49
Thank, for Task-based asynchronous pattern info, I was not paying due attention — , May 19 '14 at 21:51

What is better to return from the DataService: Status or Exceptions?

2 Answers2