Proper Error handling when we expect results from secondary functions

Question

I'm building an android application and I have some second thoughts on some error handling case.

I have a method that gets data from the internet by calling this method:

public static string StoredDatesList
{
    get => Preferences.Get(nameof(StoredDatesList), string.Empty);
    set => Preferences.Set(nameof(StoredDatesList), value);
}
public static async Task<string> GetDraws(Uri url, string date)
{
    Dictionary<string, string> StoredDates = new Dictionary<string, string>();
    StoredDates = JsonConvert.DeserializeObject<Dictionary<string, string>>(StoredDatesList);
    var contents = string.Empty;
    HttpClient client = new HttpClient();

    if (StoredDates != null)
        if (StoredDates.ContainsKey(date))
        {
            contents = StoredDates[date];
        }
        else
        {
            var current = Connectivity.NetworkAccess;
            if (current != NetworkAccess.Internet)
                return null;
            client = new HttpClient();
            contents = await client.GetStringAsync(url);
            var res2 = JsonConvert.DeserializeObject<RootObject>(contents.ToString());
            if (180 == res2.content.Count)
            {
                StoredDates.Add(date, contents);
                StoredDatesList = JsonConvert.SerializeObject(StoredDates, Formatting.Indented);
            }
        }
    else
    {
        StoredDates = new Dictionary<string, string>();

        contents = await client.GetStringAsync(url);
        var res2 = JsonConvert.DeserializeObject<RootObject>(contents.ToString());
        if (180 == res2.content.Count)
        {
            StoredDates.Add(date, contents);
            StoredDatesList = JsonConvert.SerializeObject(StoredDates, Formatting.Indented);
        }
    }
    return contents;
}

the if statement current != NetworkAccess.Internet checks if internet is available. When internet is not available I return null and I check if the data is null and display a message(error, internet is not available etc).
I find this approach very bad and I'm trying to think how is the proper way to handle this. I cannot show a message to the user from the GetDraws() function.

Maybe the correct way for this approach is to have a public variable like bool internetError = false; and to make if false every time I call GetDraws(), make it true if internet is not available and check its state after GetDraws()?
Or should I return as result of GetDraws() the error and check first if the result match of any errors?

Internet connection is not necessary every time GetDraws() is used and that is why I'm not checking before I called this function for internet connection

EDIT: My implementation i decided to be this:

    #region Setting Error Constants
    public const string Common_Error_NoInternetConnection = "Error_NoInternetConnection";
    #endregion

static MainApplication ApplicationState = (MainApplication)Application.Context;

        public static string StoredDatesList
        {
            get => Preferences.Get(nameof(StoredDatesList), string.Empty);
            set => Preferences.Set(nameof(StoredDatesList), value);
        }
        public static async Task<string> GetDraws(Uri url, string date)
        {
            var StoredDates = JsonConvert.DeserializeObject<Dictionary<string, string>>(StoredDatesList);
            var contents = string.Empty;

            if (StoredDates == null)
                StoredDates = new Dictionary<string, string>();
            if (StoredDates.ContainsKey(date))
            {
                contents = StoredDates[date];
            }
            else
            {
                if (IsInternetEnabled())
                    contents = await DownloadResults(url, date, StoredDates, contents).ConfigureAwait(false);
                else
                    return Settings.Common_Error_NoInternetConnection;
            }

            return contents;
        }
        private static bool IsInternetEnabled()
        {
            return Connectivity.NetworkAccess == NetworkAccess.Internet;
        }
        private static async Task<string> DownloadResults(Uri url, string date, Dictionary<string, string> StoredDates, string contents)
        {
            using (var client = ApplicationState.GlobalHTTPClient)
            {
                contents = await client.GetStringAsync(url).ConfigureAwait(false);

                var res2 = JsonConvert.DeserializeObject<RootObject>(contents);
                if ((int)DailyDrawsEnum.AllDraws == res2.content.Count)
                {
                    StoredDates.Add(date, contents);
                    StoredDatesList = JsonConvert.SerializeObject(StoredDates, Formatting.Indented);
                }

                return contents;
            }
        }

public class MainApplication : Application
    {
        // Global objects to share data between activities
        public MainApplication(IntPtr handle, JniHandleOwnership transer)
          : base(handle, transer)
        {
        }
        
        public HttpClient GlobalHTTPClient { get; } = new HttpClient(new HttpRetryMessageHandler(new HttpClientHandler()));
}

public class HttpRetryMessageHandler : DelegatingHandler
    {
        public HttpRetryMessageHandler(HttpClientHandler handler) : base(handler) { }

        protected override Task<HttpResponseMessage> SendAsync(
            HttpRequestMessage request,
            CancellationToken cancellationToken) =>
            Policy
                .Handle<HttpRequestException>()
                .Or<TaskCanceledException>()
                .OrResult<HttpResponseMessage>(x => !x.IsSuccessStatusCode)
                .WaitAndRetryAsync(3, retryAttempt => TimeSpan.FromSeconds(Math.Pow(3, retryAttempt)))
                .ExecuteAsync(() => base.SendAsync(request, cancellationToken));
    }

And when internet is not available i return a string error code which i check if exist and if yes, i display a message to the user:

contents = await GetDrawsFunctions.GetDraws(url, dates[datesPos]);
                    if (contents == Settings.Common_Error_NoInternetConnection)
                    {
                        ((MainActivity)Activity).ShowNoInternetConnectionSnackbarMessage();
                        return -1;
                    }

Answer 1

Thanks for clarifying the requirements, let's say they are:

• First call to GetDraws, no cached dates, has internet, download dates into cache, return date.

• First call to GetDraws, no cached dates, no internet, indicate error.

• First call to GetDraws, cached dates, download and store new cache of dates, return dates.

Let's go with that. You mention "old dates does not require to be downloaded again or refreshed, only new ones are being downloaded.' so a calls the cache is refreshed.

Okay, so you can see there are a few actions going on and they have all been placed into the one method. thinking SRP. They are related but the code meshes them together. So, I'd recommend separating out the actions into methods.

Here is a rework of the code that demonstrates how that would look like.

Now the question of how to deal with error. It's difficult because null says nothing about the failure. Raising an exception is overkill although, you see it depends on what is call this code. This isn't really the code that we need to see to help with error handling. We'd need to see what uses the null content. Let's see that code and maybe something will come to mind.

Here's the code reimagined.

class Draw
{
    public string Get(Uri uri, string date)
    {
        // load cached data, that may or may not have the key
        // if no cached data, then use empty dictionary with no keys
        Dictionary<string, string> StoredDates = this.GetStoredDates();

        if (!StoredDates.ContainsKey(date))
        {
            if (IsInternetEnabled)
            {
                // of course huge assumption here
                // the internet is enabled, but does it work!
                // is the website there?
                StoredDates.Add(this.DownloadContent(uri, date));
                this.CacheStoredDates(StoredDates);
            }
            else
            {
                // what to do here, no stored data, no way to get data
                // null content for the draws on that date? 
                // raising an exception doesn't seem correct this is not exception circumstances
            }

        }
        return StoredDates[date];
    }

    private Dictionary<string, string> GetStoredDates()
    {
        // if null then return an empty dictionary
        return JsonConvert.DeserializeObject<Dictionary<string, string>>(StoredDatesList) ? new Dictionary<string, string>();
    }

    private void CacheStoredDates(Dictionary<string, string> StoredDates)
    {
        StoredDatesList = JsonConvert.SerializeObject(StoredDates, Formatting.Indented);
    }

    private bool IsInternetEnabled()
    {
        return Connectivity.NetworkAccess != NetworkAccess.Internet;
    }

    private KeyValuePair<string, string> DownloadContent(Uri uri, string date)
    {
        // have a look at the Polly framework

        HttpClient client = new HttpClient();
        var contents = await client.GetStringAsync(url);
        var result = JsonConvert.DeserializeObject<RootObject>(contents.ToString());

        // why 180? what does it mean, make it a constant with a variable name.
        if (180 == result.content.Count)
        {
            // maybe return a key value pair here instead. Why? 
            // to reduce the side effects in your code, 
            // side effect smell, updating state from lots of places
            //  makes it difficult to parallelize later 
            return new KeyValuePair<string, string>(date, contents);
        }
        else
        {
            // what if it is not 180?
            return new KeyValuePair<string, string>(date, "let's assume that this is fine.");
        }
    }
}

Update:

To clarify the use of KeyValuePair is purely option. Previously, the UpdateContent() method updated data in the class. This creates a direct dependency between the class and the UpdateContent() method. Returning a value, whatever it's type, will allow the UpdateContent() method be movable.

Okay errors. You have control over the code that checks the error; that's good. If this was an API then maybe you wouldn't be able to change the error code as it would have breaking changes.

Checking null, not good as it implicitly says something that should be made explicit. There is a design pattern called a NullObject that might work here. The idea of using an empty collection rather than null is an example of NullObject at work; the code doesn't need to check for null it just loops over a list with no items.

https://en.wikipedia.org/wiki/Null_object_pattern

I don't think NullObject would work as the code deals with strings. NullObject for string is string.Empty with is almost the same as null in this context.

Using a predefined string would work, no doubt. Let's think about some other options.

There is a pattern in .NET called the try method. You may have seen it.

bool success = int.TryParse(input, out int value)
if(!success) 

You could have a TryGetDraw() method.

    public bool TryGetDraw(Uri uri, string date, out string content)
    {
        // load cached data, that may or may not have the key
        // if no cached data, then empty dictionary with no keys
        Dictionary<string, string> StoredDates = this.GetStoredDates();

        if (!StoredDates.ContainsKey(date))
        {
            if (IsInternetEnabled)
            {
                StoredDates.Add(this.DownloadContent(uri, date));
                this.CacheStoredDates(StoredDates);
            }
            else
            {
                // what to do here, no stored data, no way to get data
                // null content for the draws on that date? 
                // raising an exception doesn't seem correct this is not exception circumstances
                content = string.Empty;
                return false;
            }

        }

        content = StoredDates[date];
        return true;
    }

Called like this...

bool success = draws.TryGetDraw(uri, date, out string content)
if(!success) // show message box

That's removes the need to check content completely, no special strings; however, it does cause information about the reasons why TryGetDraw failed.

Maybe, the content string could contain a human readable reason for the failure. Maybe we can improve the pattern with an enumeration.

    enum Outcome
    {
        Success,
        InternetDisabled,
        RequestTimeout,
        InternalServerError,
        Unknown
    }

    public Outcome TryGetDraw(Uri uri, string date, out string content)
    {
        if (!StoredDates.ContainsKey(date))
        {
            if (IsInternetEnabled)
            {
                try
                {

                }
                // just be sure this makes sense
                catch (HttpRequestException ex)
                {
                    return Outcome.InternalServerError;
                }
            }
            else
            {
                content = string.Empty;
                return Outcome.InternetDisabled;
            }
        }

        content = StoredDates[date];
        return Outcome.Success;
    }

Called like this...

Outcome outcome = draws.TryGetDraw(uri, date, out string content)
if(outcome == Outcome.InternetDisabled) // check wifi is on msg
if(outcome == Outcome.InternalServerError) // try again later msg

That could lead to a mass of Outcomes and if statements everywhere that would need to maintained so treat this approach with care. There's no silver bullet.

The two try methods are the same, just depends on whether you want the code being called to know the error messages (error messages in TryGetDraw method) or the code doing the calling to know the error messages (in the Form).

One last thing, since you are calling out over the internet the different errors you will get will be more than Internet is disabled. The internet maybe enables but the network might be down, the web server may be down, the web server might be too busy, it might have an internal error, the data might not be at that url, hopefully it's accessed over TLS... so anyway the nuget package Polly will allow your code to be more resilient to this uncertainty.

https://en.wikipedia.org/wiki/Fallacies_of_distributed_computing

https://docs.microsoft.com/en-us/dotnet/architecture/microservices/implement-resilient-applications/

https://docs.microsoft.com/en-us/azure/architecture/patterns/retry

https://docs.microsoft.com/en-us/dotnet/architecture/microservices/implement-resilient-applications/implement-http-call-retries-exponential-backoff-polly