Consider this stack trace from an exception thrown by Entity Framework 6.1.3:
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 |
System.InvalidOperationException : Sequence contains no matching element at System.Linq.Enumerable.Single[TSource](IEnumerable`1 source, Func`2 predicate) at System.Data.Entity.Utilities.DbProviderManifestExtensions.GetStoreTypeFromName(DbProviderManifest providerManifest, String name) at System.Data.Entity.ModelConfiguration.Configuration.Properties.Primitive.PrimitivePropertyConfiguration.ConfigureColumn(EdmProperty column, EntityType table, DbProviderManifest providerManifest) at System.Data.Entity.ModelConfiguration.Configuration.Properties.Primitive.PrimitivePropertyConfiguration.Configure(EdmProperty column, EntityType table, DbProviderManifest providerManifest, Boolean allowOverride, Boolean fillFromExistingConfiguration) at System.Data.Entity.ModelConfiguration.Configuration.Properties.Primitive.PrimitivePropertyConfiguration.<>c__DisplayClass4.<Configure>b__3(Tuple`2 pm) at System.Data.Entity.Utilities.IEnumerableExtensions.Each[T](IEnumerable`1 ts, Action`1 action) at System.Data.Entity.ModelConfiguration.Configuration.Properties.Primitive.PrimitivePropertyConfiguration.Configure(IEnumerable`1 propertyMappings, DbProviderManifest providerManifest, Boolean allowOverride, Boolean fillFromExistingConfiguration) at System.Data.Entity.ModelConfiguration.Configuration.Types.StructuralTypeConfiguration.ConfigurePropertyMappings(IList`1 propertyMappings, DbProviderManifest providerManifest, Boolean allowOverride) at System.Data.Entity.ModelConfiguration.Configuration.Types.EntityTypeConfiguration.ConfigurePropertyMappings(DbDatabaseMapping databaseMapping, EntityType entityType, DbProviderManifest providerManifest, Boolean allowOverride) at System.Data.Entity.ModelConfiguration.Configuration.Types.EntityTypeConfiguration.Configure(EntityType entityType, DbDatabaseMapping databaseMapping, DbProviderManifest providerManifest) at System.Data.Entity.ModelConfiguration.Configuration.ModelConfiguration.ConfigureEntityTypes(DbDatabaseMapping databaseMapping, ICollection`1 entitySets, DbProviderManifest providerManifest) at System.Data.Entity.ModelConfiguration.Configuration.ModelConfiguration.Configure(DbDatabaseMapping databaseMapping, DbProviderManifest providerManifest) at System.Data.Entity.DbModelBuilder.Build(DbProviderManifest providerManifest, DbProviderInfo providerInfo) at System.Data.Entity.DbModelBuilder.Build(DbConnection providerConnection) at System.Data.Entity.Internal.LazyInternalContext.CreateModel(LazyInternalContext internalContext) at System.Data.Entity.Internal.RetryLazy`2.GetValue(TInput input) at System.Data.Entity.Internal.LazyInternalContext.InitializeContext() at System.Data.Entity.Internal.InternalContext.ForceOSpaceLoadingForKnownEntityTypes() at System.Data.Entity.DbContext.System.Data.Entity.Infrastructure.IObjectContextAdapter.get_ObjectContext() |
Lucky for me, I know which tiny change I made to the code I am working with to induce the exception, and therefore I am pretty confident that I know what caused it. Had I not been so fortunate, this stack trace would have provided almost no help at all with diagnosing what is actually wrong.
Let’s try and decompile the topmost method, GetStoreTypeFromName, which is the one that apparently calls .Single(..) on a sequence without any matches. It looks like this:
|
1 2 3 4 5 6 7 |
internal static class DbProviderManifestExtensions { public static PrimitiveType GetStoreTypeFromName(this DbProviderManifest providerManifest, string name) { return providerManifest.GetStoreTypes().Single<PrimitiveType>((Func<PrimitiveType, bool>) (p => string.Equals(p.Name, name, StringComparison.OrdinalIgnoreCase))); } } |
This, Ladies And Gentlemen, is in my humble opinion almost unforgivable: It is simply not OK to call .Single(..) on stuff without re-throwing the exception along with information on what we were looking for.
A much improved version of the method would have looked like this:
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 |
internal static class DbProviderManifestExtensions { public static PrimitiveType GetStoreTypeFromName(this DbProviderManifest providerManifest, string name) { try { return providerManifest.GetStoreTypes() .Single<PrimitiveType>(p => string.Equals(p.Name, name, StringComparison.OrdinalIgnoreCase)); } catch(Exception exception) { throw new ArgumentException($"Could not find PrimitiveType named '{name}'", exception); } } } |
this way not losing the original exception, still providing information from the callsite telling the caller what’s wrong.
An even better version of the method would provide me with a list of available “store types”, which often provides some great hints about what is wrong:
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 |
internal static class DbProviderManifestExtensions { public static PrimitiveType GetStoreTypeFromName(this DbProviderManifest providerManifest, string name) { var storeTypes = providerManifest.GetStoreTypes(); try { return storeTypes .Single<PrimitiveType>(p => string.Equals(p.Name, name, StringComparison.OrdinalIgnoreCase)); } catch(Exception exception) { throw new ArgumentException($"Could not find PrimitiveType named '{name}' among the following store types: {string.Join(", ", storeTypes.Select(p => $"'{p.Name}'"))}", exception); } } } |
The next method in the stack trace is called ConfigureColumn, and sadly if you decompile the method, it will be just as cheap with regards to helping us know what was wrong, even though it has all kinds of valuable things to tell us like:
- which type was it trying to map
- which property could not be mapped
- which column in the database was it trying to map to
etc. etc. etc.
Please don’t write code like that.
I agree, but I think that the reason that code like that gets written is that it’s easier to write the ‘bad’ code than it is to write the proper code. C# (and Java for that matter) lets you write bad code.
In e.g. Haskell, the function corresponding to Single is Data.List.find, which doesn’t return a single element, but rather a ‘Maybe element’. It’s possible that the function will return an element, but it also may return Nothing, and all callers are forced by the compiler to deal with both cases. It’s much harder to write bad code in Haskell, because the bad code is not going to compile.
F# occupies a middle ground. It enables you to write worse code than Haskell, but in general you have to go out of your way to write as bad code as C# lets you do.
BTW, the call to GetStoreTypes could return null, so that should be handled as well. This makes the exception handling even more complicated. Your current suggestion with a try/catch block could lead to a misleading error messages, because it’ll say that the problem was that it couldn’t find a particular element, while the actual exception was a NullReferenceException.
I tend to use .SingleOrDefault in my code and then test for the null and throw an exception.
I do like this approach but I think I’d make some helpful extension methods just to reduce the ceremony involved. Perhaps (NB: pseudocode!)
public static T SingleWithEx(this IEnumerable enumerable, Predicate pred, string exceptionMessage) {
try {
return enumerable.Single(pred);
} catch(err) {
throw new ArgumentException(exceptionMessage, err);
}
}
This could also elegantly handle the null case as the awesome Mr Seemann points out.
If the message was expensive then I’d make it a callback.
The extension could also take a Func to help with the string.Join() calculations you did.
Sure it might be a little less efficient, but this should be the unhappy, infrequent code path anyway.