3
\$\begingroup\$

I'm using a DataContext to insert an object. Currently it's working, but I'm feeling things could be done better. Method is called from UI (Using Caliburn Micro Binding). IsBusy is a property used by the UI (Xceed Extended WPF Toolkit) to display a wait message.

Repository inherits from DataContext (using Entity Code First).

public async void AddTemplate()
{
    IsBusy = true;

    await Task.Run(() => {
        using (var repo = new Repository())
        {
            var template = new Template() { ... };
            try
            {
                repo.Templates.Add(template);
                repo.SaveChanges();
                this.TryClose(true);
            }
            catch (SqlException ex)
            {
                _util.Log.AddException(Exceptions.DB_EXC, ex.Message);
            }
            catch (EntityException ex)
            {
                _util.Log.AddException(Exceptions.EF_EXC, ex.Message);
            }
            catch (Exception ex)
            {
                _util.Log.AddException(Exceptions.US_EXC, ex.Message);
            }
            finally
            {
                IsBusy = false;
            }
        }
        this.TryClose(false);
    });
}

I'm feeling odd about covering using with the await Task.Run since I can also await for repo.SaveChangesAsync(), but then IsBusy will not change (and the Wait message will not be made visible).

\$\endgroup\$
4
\$\begingroup\$

You should be able to await the repo.SaveChangesAsync() method.

public async Task AddTemplateAsync()
{
    var template = new Template {...};

    using (var repo = new Repository())
    {
        try
        {
            IsBusy = true;
            repo.Templates.Add(template);
            await repo.SaveChangesAsync();
        }
        catch {...}
        finally
        {
            IsBusy = false;
            TryClose();
        }
    }
}

Since AddTemplate() is an async method, it should be called AddTemplateAsync() and return a Task. Take a look at these best practices for more details. You might want to consider passing in a CancellationToken depending on your requirements.

I would also advise against swallowing the base Exception type for reasons explained here.

\$\endgroup\$
1
\$\begingroup\$

(Old question, but that's no reason not to add another answer).

One of the main advantages of the async/await pattern is that it allows for code locality - that is, to write straightforward code, line by line, that doesn't have to be structured like Task's ContinueWith.

This is especially useful with async/await's support for try/catch/finally, allowing you to put clean-up code in the same level as your set-up code, namely - putting IsBusy=false in a top-level try/finally block. That way your UI-handling code is at the top level, and the repository/business logic code in the inner task, and can even be extracted to a separate method that knows nothing of the UI.

public async void AddTemplate() 
{
    try 
    {
       IsBusy = true;
       await Task.Run(() => { /* repository/BL code only */ });
    }
    finally
    {
       IsBusy = false;
    }
}
\$\endgroup\$
1
\$\begingroup\$

Your question has been answered in my opinion, but this is Code Review and if be remiss to not point out that you might as well just be swallowing those exceptions. Sure, logging them off is useful to a point, but your user is going to continue on, blissfully unaware that their data has not been saved. Which means that you will likely be unaware of the issue as well. Oh sure, you've got the log, but do you really ever look at it?

I'm betting you don't.

Speaking of the exceptions, I don't see a great benefit to catching SqlException and EntityException, only to turn around and swallow every possible exception that could happen. If you're going to catch Exception, you might as well remove the other two catches.

But I need to know which type it was!

Yes, that's useful and that's why your logger is just plain wrong. Don't log the message, log the stack trace. You'll thank yourself later.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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