5
\$\begingroup\$

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;
                    }
\$\endgroup\$
9
  • \$\begingroup\$ Please add a description of what GetDraws() is to accomplish (the source code would have been a favourable place). \$\endgroup\$
    – greybeard
    Sep 5, 2020 at 10:53
  • \$\begingroup\$ I have post the source code of GetDates() get dates return a list of strings if the data exists on the DB or download them if they are not available. If they are not available and internet connection is not available im trying to figure out the best way to handle it and display an error to the user. \$\endgroup\$
    – CDrosos
    Sep 5, 2020 at 11:00
  • \$\begingroup\$ My question is: if another function is calling GetDates() how i should handle the errors. \$\endgroup\$
    – CDrosos
    Sep 5, 2020 at 11:01
  • \$\begingroup\$ HttpClient is intended to be instantiated once per application, rather than per-use. And you don't need to call .ToString() for string members because string.ToString() has no sense if you pass no arguments there. \$\endgroup\$
    – aepot
    Sep 7, 2020 at 6:08
  • 1
    \$\begingroup\$ Oh i haven't thought about it. I can have only one in the application state and use only that one everywhere \$\endgroup\$
    – CDrosos
    Sep 7, 2020 at 6:11

1 Answer 1

3
\$\begingroup\$

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

\$\endgroup\$
6
  • \$\begingroup\$ @CDrosos you might want to clarify with the business or user of the system what they expect to happen in the case of an network outage, what would they want to happen if the data wasn't there. User experience people as well; if there are any. \$\endgroup\$
    – null
    Sep 5, 2020 at 11:26
  • \$\begingroup\$ great changes thanks, so with keyvaluepair it will be impossible for a date to exist twice in the list? while in my implementation it might be possible? 180 are all the results that can be in one day, so if they are 180 i will store them into the DB, if not then lets try to download them again.sometimes it is ok to have less than 180 but in the end of a day they should be 180 so i only store them if the results are 180 (we can call the 180 as of DailyDraws). What if i have a table with constrant strings of errors and when internet is not available i return the string error code? \$\endgroup\$
    – CDrosos
    Sep 5, 2020 at 13:08
  • 1
    \$\begingroup\$ @CDrosos, the keyvaluepair was used because the code to download from the internet really belongs elsewhere, and passing something back rather than updating directly allows that method to be moved, it's impossible to test this code so injecting a 'downloader' would be better in the long run. Have a look at the Polly framework, it allows automatic retries of a request. If the internet isn't on then it's good to think why and for how long. I'll be online later and have will have a look at the updates. \$\endgroup\$
    – null
    Sep 5, 2020 at 14:34
  • 1
    \$\begingroup\$ @CDrosos updated, couple of options for you. \$\endgroup\$
    – null
    Sep 6, 2020 at 9:31
  • 1
    \$\begingroup\$ I have update the question with the code i use now from your ideas and aepot notes, i believe is an acceptable workaround the way i check if internet connection is available. \$\endgroup\$
    – CDrosos
    Sep 7, 2020 at 16:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.