4
\$\begingroup\$

I am implementing my own UnitOfWork for an EntityFramework-DataContext.

I want to call SaveChangesAsync and when this succeeds, clear a list. Is my approach with ContinueWith correct?

public async Task<int> CommitAsync(int? commandTimeout = null, bool ignoreCustomLogic = false)
{
    try
    {
        return await DataContext.SaveChangesAsync().ContinueWith((antecedent) =>
        {
            if (!antecedent.IsFaulted)
            {
                _addedEntities.Clear();
                _changedEntities.Clear();
                _deletedEntities.Clear();
            }
            return antecedent.Result;
        });
    }
    catch (DbEntityValidationException dbEx)
    {
        RethrowException(dbEx);
        return default(int);
    }
}

private void RethrowException(DbEntityValidationException dbEx)
{
    string message = "ValidationExceptions: " + Environment.NewLine;
    foreach (var validationResult in dbEx.EntityValidationErrors)
    {
        foreach (var validationError in validationResult.ValidationErrors)
        {
            message += string.Format("Property: {0} Error: {1}{2}", validationError.PropertyName, validationError.ErrorMessage, Environment.NewLine);
        }
    }
    throw new Exception(message, dbEx);
}
\$\endgroup\$
6
\$\begingroup\$

I want to call SaveChangesAsync and when this succeeds, clear a list.

You don't need ContinueWith, you can just write it like this

public async Task<int> CommitAsync()
{
    var result = await DataContext.SaveChangesAsync();
    _addedEntities.Clear();
    _changedEntities.Clear();
    _deletedEntities.Clear();
    return result;
}

In the original code, the catch block will never run.

Consider what happens when SaveChangesAsync throws a DbEntityValidationException: antecedent.Result will throw an AggregateException wrapping the DbEntityValidationException.

It's poor practice to throw an Exception as it means client code cannot be specific about the exceptions it catches. I would suggest changing RethrowException to a method that constructs the exception message you want, then calling it like this:

public async Task<int> CommitAsync()
{
    try
    {
        var result = await DataContext.SaveChangesAsync();
        _addedEntities.Clear();
        _changedEntities.Clear();
        _deletedEntities.Clear();
        return result;
    }
    catch (DbEntityValidationException e)
    {
        var exceptionMessage = GetDbEntityValidationExceptionMessage(e);
        throw new DbEntityValidationException(exceptionMessage, e);
    }
}

This also removes the return default(int); which is just there to appease the compiler.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Alternatively, you don't need the await keyword. One or the other could be removed. I prefer mjolka's method, as it is much easier to read. \$\endgroup\$ – Nick Udell May 26 '15 at 12:02
  • 1
    \$\begingroup\$ Your refactor does not take the case of antecedent.IsFaulted into consideration like the original code did. \$\endgroup\$ – psaxton May 27 '15 at 6:13
  • \$\begingroup\$ @psaxton antecedent is either faulted, cancelled, or ran successfully to completion. Since we're not passing a CancellationToken to SaveChangesAsync, I don't see how it can be cancelled. So the lists will only be cleared in the original code if the SaveChangesAsync ran successfully to completion -- which is the same in the code I posted. \$\endgroup\$ – mjolka May 27 '15 at 9:58
  • \$\begingroup\$ I suppose that makes sense. I've not used ContinueWith before and was not aware of that; I just see that the original is checking for IsFaulted and catching as well. \$\endgroup\$ – psaxton May 27 '15 at 13:17
2
\$\begingroup\$

Something @mjolka corrected but didn't comment on was the unused parameters in your SaveChangesAsync method signature.

If method parameters are not used, don't add them. This is an example of the YAGNI principle. If you later implement the functionality you anticipate using the parameters for, you have locked yourself into an implementation which may not fit your future needs. You also risk breaking consumers expecting the current behavior.

If unused parameters are needed to satisfy an interface definition, seriously consider if your object fits the intent of the interface. This is commonly considered design by contract. Otherwise, implement the method to satisfy the interface contract. As a last resort, document the discrepancy in XmlDoc comments if the the object satisfies the intent of the interface and a proper implementation is not reasonable. In your example, commandTimeout should be implemented. A consumer relying on proper behavior could have disastrous consequences. There doesn't seem to be any reasonable custom logic possible in this implementation -- indicate ignoreCustomLogic is ignored if it can't be removed entirely.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I edited my post. The parameters are not unused but not part of my question. I had the intension to remove them, but forgot to do so... \$\endgroup\$ – user2900970 May 28 '15 at 7:47

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.