C# should I use try catch within a try catch to handle response? is it a bad idea?

Question

My code handles some rest APIs (GET methods). This GET method, returns a response like this:

{"error":0,"Logs":[{"LoggerIdx":"91","OfficeID":"MIA1A0955","Agent":"581A78AD"}]}

and if the query doesn't find anything, returns:

{"error":0,"Logs":[{"No values found"}]}

The code I'm using call this API to retrieve the values and show a report is:

private string uri = "http://localhost";

    public async Task<List<T>> GetWSObjects<T>(string uriActionString)
    {
        return new List<T> { await this.GetWSObject<T>(uriActionString) };
    }

public async Task<T> GetWSObject<T>(string uriActionString)
    {
        T returnValue =
            default(T);
        try
        {
            using (var client = new HttpClient())
            {
                client.BaseAddress = new Uri(uri);                    
                client.DefaultRequestHeaders.Accept.Clear();
                client.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));
                HttpResponseMessage response = await client.GetAsync(uriActionString);
                response.EnsureSuccessStatusCode();

                returnValue = JsonConvert.DeserializeObject<T>(((HttpResponseMessage)response).Content.ReadAsStringAsync().Result);
            }
            return returnValue;
        }
        catch (Exception e)
        {
            throw (e);
        }
    }

returnValue tries to fill my model with the values in the response. However, when the response only contains "No values found" it breaks(obviously). My question is, should I place a try-catch within this try-catch to handle this behavior? The problem is that the whole exception is being shown to the user, not only "Not values found". Suggestions? My model is:

    public class BuildingReportModel
{
    public string message1 { get; set; }
    public Log[] Logs { get; set; }        
}

public class Log
{
    public string ProdLoggerIdx { get; set; }
    public string OfficeID { get; set; }     
    public string Agent { get; set; }
}

you are simply rethrowing the error. you can even get rid of the outer try catch all together — Steve, Dec 15 '16 at 21:27
@Steve I thought about it, but what about catching other real errors, like bad input, sql ? — Rolando F, Dec 15 '16 at 21:29
You should take a look at this [answer](http://stackoverflow.com/a/14974163/6936343). — Tony_KiloPapaMikeGolf, Dec 15 '16 at 21:35
Try catch inside a try catch makes my throw up in my mouth just a little. Guy at my work put them in the finally block, so it could be worse.... — Trey, Dec 15 '16 at 21:47

Yaser · Answer 1 · 2016-12-15T22:14:27.720

3

The best way of handling that would be to check the response before parsing it:

var responseString = await ((HttpResponseMessage)response).Content.ReadAsStringAsync();
if(response.ToLower().Contains("no values found")) {
    //do something here like returning an empty model
}
else
{
    returnValue = JsonConvert.DeserializeObject<T>(responseString);
}

edited Dec 15 '16 at 22:14

answered Dec 15 '16 at 21:31

Yaser

5,609
1
15
27

1

.Result is a really good way to shoot yourself in the foot....potential deadlock if current SynchronizationContext is not null – Steve Dec 15 '16 at 22:06
this way makes sense, however, this "response" doens't have ToLower, neither Contains methods – Rolando F Dec 15 '16 at 22:14
sorry didn't see it, had to refresh to see that last comment – Rolando F Dec 15 '16 at 22:20

geekzster · Answer 2 · 2017-01-03T15:12:43.027

In your catch block, log or otherwise capture the exception, but don't throw if the return value of this method is being displayed (which from what you posted seems like it is). Then write a friendly message to your model (I'm assuming that is what the string message1 property is for? If not you could possibly add a property to it for just such purposes). That way your user will see something that you wrote which is much more likely to be understood than the details of the exception.

As far as doing another try catch - you could, but it isn't necessary. Whatever exceptions are thrown by JsonConvert.DeserializeObject will ultimately be of type Exception and will be caught by your catch. If you want to catch a specific exception type, put another catch statement before your Exception catch block, and treat it in the same fashion.

Articles and opinions about how to re-throw (which is what your current code does) are all over the place - do some research and find the best pattern for your situation.

Also, the answer provided by @Yaser is a good idea as well - if you can prevent the exception in the first place, the rest is moot.

So here is an example of your posted code which I modified and commented in a little bit, which may help clarify (there is room for improvement but that is outside the scope of your question):

  public async Task<T> GetWSObject<T>(string uriActionString)
    {
        var returnValue = default(T);
        try
        {
            using (var client = new HttpClient())
            {
                client.BaseAddress = new Uri(uri);
                client.DefaultRequestHeaders.Accept.Clear();
                client.DefaultRequestHeaders.Accept.Add(
                    new MediaTypeWithQualityHeaderValue("application/json"));
                var response = await client.GetAsync(uriActionString);
                response.EnsureSuccessStatusCode();

                // this does not need a try catch, 
                // because whatever exception is thrown here 
                // will still be caught
                returnValue = JsonConvert.DeserializeObject<T>(
                    response.Content.ReadAsStringAsync().Result);
            }

        }
        catch (Exception e)
        {
            // log or otherwise capture the exception details
            // if you don't need to log/capture the e variable
            returnValue.message1 = "A user-friendly description of the problem";
        }
        //catch // could also do it this way
        //{
        //    // if you don't need to log/capture the exception,
        //    // then don't bother with the overload
        //    returnValue.message1 = "A user-friendly description of the problem";
        //}

        return returnValue;
    }

the tricky part here is that I'm passing a LIST so I can't access directly to returnValue message1, and if I do something in the catch, how can I identify each problem, in my case, that "No values found"? — Rolando F, Dec 16 '16 at 14:19
@Rolando F - I can certainly provide a suggestion, but you asked "My question is, should I place a try-catch within this try-catch to handle this behavior? The problem is that the whole exception is being shown to the user, not only "Not values found". Suggestions?" I believe I have answered your question and provided some suggestions. If you still need help I can take a closer look and provide more feedback, but I wanted to point this out. — geekzster, Dec 16 '16 at 15:03

score 0 · Answer 3 · answered Dec 16 '16 at 15:02

Not really sure if it is the best way to solve it like this, but it is working:

catch (Exception e)
        {                
            if (e.Message.ToString().Contains("No Logs"))
            {
                Exception e2 = (Exception)Activator.CreateInstance(e.GetType(), "No Logs Found ...", e);
                throw e2;                    
            }
            throw (e);
        }

I'm just verifying if the exception contains "No logs" and if is true, create a new exception and throw it. Seems to work, but is there any cons on doing it like this? any thoughts?

C# should I use try catch within a try catch to handle response? is it a bad idea?

3 Answers3