Improving LINQ expression with inner method call and switch statement

Question

I have the following class which returns an expression that then returns data. It seems a tad over complicated and I'm pretty sure LINQ has something to offer that makes my life much easier. But I simply cannot get it.

/// <summary>Provides methods and properties to query location details from a database.</summary>
internal sealed class LocationDatabaseDetailsRequest : DatabaseDetailsRequest<LocationContext, LocationDataContract, Guid>
{
 /// <inheritdoc />
 public override Func<LocationContext, LocationDataContract> Resource
 {
 get
 {
 return
 c =>
 c.Locations.Select(
 l =>
 new LocationDataContract
 {
 Id = l.Id,
 Coordinates = l.Coordinates,
 Description = l.Description,
 Name = l.Name,
 Type = l.Type,
 Details = this.GetDetails(l.Type, c)
 }).SingleOrDefault(i => i.Id == this.Identifier);
 }
 }
 private LocationDetailsDataContract GetDetails(string detailsType, LocationContext locationContext)
 {
 switch (detailsType)
 {
 case "City":
 return locationContext.Set<CityDetailsDataContract>().SingleOrDefault(i => i.Id == this.Identifier);
 case "Planet":
 return locationContext.Set<PlanetDetailsDataContract>().SingleOrDefault(i => i.Id == this.Identifier);
 case "StarSystem":
 return locationContext.Set<StarSystemDetailsDataContract>().SingleOrDefault(i => i.Id == this.Identifier);
 case "Sector":
 return locationContext.Set<SectorDetailsDataContract>().SingleOrDefault(i => i.Id == this.Identifier);
 default:
 return null;
 }
 }
}

My problem is the method call. I know which sets the data context has, but they are fluid, meaning it is certain that new sets will get added.

Can I write this query shorter and well... better?

• Pierre Menard: How does Resource fit in? Maybe remove that and add enough code so that everything compiles.
• Bruno: What does the .Set<T>() method do?
• Ruhrpottpatriot: The .Set<T> method is a method from entity framework. I generates a DbSet of type T, which contains the database data.
• svick: By "my problem", do you mean your code doesn't work?

Answer 1

public override Func<LocationContext, LocationDataContract> Resource
{
 get
 {
 return
 c =>
 c.Locations.Select(
 l =>
 new LocationDataContract
 {
 Id = l.Id,
 Coordinates = l.Coordinates,
 Description = l.Description,
 Name = l.Name,
 Type = l.Type,
 Details = this.GetDetails(l.Type, c)
 }).SingleOrDefault(i => i.Id == this.Identifier);
 }
}

This code is inefficient, because the body of your Select is being executed for every value in Locations. This is because SingleOrDefault checks every item in an enumerable for duplicates. Since you're only checking an id and the id is not changed in your Select perform your SingleOrDefault call first.

public override Func<LocationContext, LocationDataContract> Resource
{
 get
 {
 return c =>
 {
 var location = c.Locations.SingleOrDefault(i => i.Id == this.Identifier);
 return location == null ? 
 null : 
 new LocationDataContract
 {
 Id = location.Id,
 Coordinates = location.Coordinates,
 Description = location.Description,
 Name = location.Name,
 Type = location.Type,
 Details = this.GetDetails(location.Type, c)
 });
 }
 }
}

This way, the LocationDataContract is only created if there is a single location in c that matches the identifier.

Structurally, I'm wondering why you have a public read-only property returning a Func. I can see no good reason why this shouldn't be a method, although obviously the motivation for this depends on the implementation in the base class, which we cannot see in your question.

If you can modify the base class, I would recommend replacing this property with a virtual or abstract method. This property syntax instead is more obtuse for a maintenance programmer to read and is over-complex. Consider this instead:

public override LocationDataContract Resource(LocationContext context)
{
 var location = context.Locations.SingleOrDefault(l => l.Id == this.Identifier);
 return location == null ? 
 null : 
 new LocationDataContract
 {
 Id = location.Id,
 Coordinates = location.Coordinates,
 Description = location.Description,
 Name = location.Name,
 Type = location.Type,
 Details = this.GetDetails(location.Type, context)
 });
}

Additionally, the name Resource gives me no information. It might as well be called Foo. It looks like a function that grabs a location data contract from a location context based on the currently stored Id, so a name like "LocationContractForCurrentId" would make more sense.

Lastly, your usage of SingleOrDefault everywhere concerns me. If it's truly acceptable that nothing happens when no identifiers can be found, including in your context set methods, then fine, keep it that way, but if this is a way to hide from exceptions caused by invalid data or incorrect programming and to avoid Failing Fast, then that is a serious issue that would be better dealt with than hidden from.

• Ruhrpottpatriot: 1. Thanks for the answer. 2. The name resource was deliberately chosen. It has something to to with the rest of the flow and how I execute requests against my data sources (which are not all databases). I have multiple requests and naming the property based on what to do is not possible. 3.The SingleOrDefault method is only used to check if data is returned, nothing more. In other parts of the program the data is converted into another format, and if there is nothing to return from the database then the rest of the program should do nothing.