4
public class CarDTO
{
    public int CarId { get; set; }
    public string CarName { get; set; }
    public List<PolicySummaryDTO> PolicySummaries { get; set; } 
}

public class PolicySummaryDTO
{
    public long PolicyId { get; set; }
    public string PolicyName { get; set; }
    public decimal Amount { get; set; }
}

I have List<CarDTO> Cars each car has already list of policies List<PolicySummaryDTO> PolicySummaries. PolicyId is filled. But other properties like PolicyName, Amount, etc. are not. The thing I need to do is get data from DB _contex.PolicySummary and I need to complete missing fields.

I know that I could do this in this way.

  1. Get all PolicyIds var policyIds = cars.SelectMany(t => t.PolicySummaries).Select(r => r.PolicyId);

  2. Get PolicyData based on Ids var policyData = _context.PolicySummary.Where(t => policyIds.Contains(t.PolicyId));

  3. And then using foreach I can fill data.

    foreach (var car in cars)
    {
        foreach (var policy in car.PolicySummaries)
        {
            var policyDataToUse = policyData.Single(t => t.PolicyId == policy.PolicyId);
            policy.Amount = policyDataToUse.Amount;
            policy.PolicyName = policyDataToUse.PolicyName;
        }
    }
    

Everthing will works fine, but I wondering whether I can do it in more elegant way, maybe using somehow LINQ JOIN or something, or maybe my solution is totally correct?


EDIT - solution with dictionary

var policyIds = cars.SelectMany(t => t.PolicySummaries).Select(r => r.PolicyId);
var policyDict = _context.PolicySummary.Where(t => policyIds.Contains(t.PolicyId)).ToDictionary(t => t.PolicyId);

foreach (var car in cars)
{
    foreach (var policy in car.PolicySummaries)
    {
        var policyDataToUse = policyDict[policy.PolicyId];
        policy.Amount = policyDataToUse.Amount;
        policy.PolicyName = policyDataToUse.PolicyName;
    }
}
DiPix
  • 5,755
  • 15
  • 61
  • 108
  • 1
    Are you using EF? Or how do you map objects to entities? If you are using any framework or ORM there should be a 'correct' way to get your properties. Please edit your question and give us more details on how your project is setup. – nilsK Aug 14 '18 at 08:08
  • @nilsK Actually there is a tag 'entity-framework' in this question. – Fabjan Aug 14 '18 at 08:22
  • 1
    @Fabjan Yes, I added after his comment :) – DiPix Aug 14 '18 at 08:29

2 Answers2

1

So first of all, why is the data not filled? You should be able to fill out the data in any related entities during the actual Car fetch from the DB using .Include.

var carEntity = _context.Cars.Where(predicate).Include(c => c.PolicySummaries).Single();

Secondly, your code suffers from a serious performance issue: policyData is an IQueryable. Every time you do policyData.Single, you send a new query to the database, look through all policies, filter the ones that have their id in policyIds and choose the appropriate one. This will be incredibly inefficient if there are many policies - every foreach iteration is a new query! What you should do is probably do something like:

var policyData = _context.PolicySummary.Where(t => policyIds.Contains(t.PolicyId)).ToList();

To fetch all related policies. This could also be inefficient if there are a lot of shared policies and you end up loading most of them at some point anyway, so if there's not like a billion policies in the DB this:

var policies = _context.PolicySummary.ToList();

could work even better. It's a time/memory tradeoff at this point, but please note that any of the above will be better than the current query-overload foreach you have.

V0ldek
  • 9,623
  • 1
  • 26
  • 57
  • It's because `PolicySummaryDTO` has one more List inside. And this `PolicyId` from `PolicySummaryDTO` is filled based on the first record from that list. It's complicated case, I know it. I don't wanted to complicate this question as well. – DiPix Aug 14 '18 at 08:21
  • You still should be able to do that in a db fetch, but if you want it done separately then at least adhere to my second point. It should be alright efficiency-wise then. – V0ldek Aug 14 '18 at 08:29
  • Don't use `Single()` because it can throw exception if there is more than one item or no items at all. Use `SingleOrDefault()`. For more information: https://stackoverflow.com/questions/5619283/should-i-use-single-or-singleordefault-if-there-is-a-chance-that-the-element; http://www.technicaloverload.com/linq-single-vs-singleordefault-vs-first-vs-firstordefault/ P.S: I do not have any affiliation with this site. – Lewis86 Aug 14 '18 at 08:48
  • 1
    Using `Single` or `SingleOrDefault` depends on what you want to achieve. If you're certain at that point in code that there _should_ be a record in the database with given `Id`, then use `Single`. I usually prefer my repositories to throw exceptions rather than return `null`s for non-exisiting entities, but it depends. Anyway, saying _don't use_ isn't a good advice. Both have their appropriate use cases. – V0ldek Aug 14 '18 at 08:50
  • It is a bad policy. Let's say you are working on an application that cannot stop working, you prefer that the application stops working? If it gives a default value, then you know that smth isn't right and you need to fix it. – Lewis86 Aug 14 '18 at 09:00
  • 1
    Always crash early. It's way better to throw an exception and handle it - log an error, tell the user something went wrong and move on. If it shouldn't ever happen - it's an exception. If a default value is a sensible, usable value - return it. "Smth isn't right" always infers an exceptional situation that should be raised ASAP. – V0ldek Aug 14 '18 at 09:11
  • 1
    @Lewis86 - throw exception when error is an **exception** for your business logic. When only one record with particular id must exists in database, then `Single` would more comprehensible approach. Usually you catch all errors on the top layer of your application (UI or Web API), log them and display/return friendly error message for the user. Logged exception provide enough information about error cause. As a bonus rest of application don't need to deal "default" values, which will simplify code base. – Fabio Aug 14 '18 at 11:19
0

Because you already have existed DTO objects(not entities), you will not be able to effectively use of Join with EntityFramework.
You need to load missing data and then update existed car objects with it.

I think simpler way will be replace list of policies with objects retrieved from database.

var allPolicies = _context.PolicySummary
                          .Where(policy => policyIds.Contains(policy.PolicyId))
                          .Select(group => new PolicySummaryDTO
                          {
                              PolicyId = group.Key,
                              PolicyName = group.First().PolicyName,
                              Amount = group.First().Amount                  
                          })
                          .ToDictionary(policy => policy.PolicyId);

foreach (var car in cars)
{
    car.Policies = car.Policies.Select(p => p.PolicyId)
                               .Where(allPolicies.ContainsKey)
                               .Select(policyId => allPolicies[policyId])
                               .ToList();
}

Using Dictionary will keep updating code simpler and reduce amount of operations to execute, less loops.

Note: approach above assumes that all policies Id's exist in the database. If not, these will be removed from the correspondent car.Policies property. You can remove Where(allPolicies.ContainsKey) line and an exception will be thrown if some policies are missing.

Fabio
  • 31,528
  • 4
  • 33
  • 72
  • There is no point in assigning a new collection to `car.Policies` when it's possible to flatten them and update a few fields in each element furthermore the `.ToList()` call will return a new collection which might or might not be a desirable behavior here – Fabjan Aug 14 '18 at 08:45
  • @Fabjan, as you said it _might or might not be a desirable behavior_ ;). As a benefit, you don't need to change updating code in case you will add/remove new properties to PolicySummaryDTO class. – Fabio Aug 14 '18 at 08:53
  • Part with dictionary returns me `System.ArgumentException: 'An item with the same key has already been added. Key: 0'` – DiPix Aug 14 '18 at 10:59
  • @DiPix, you can group items by `PolicyId` before creating a Dictionary, check updated answer. – Fabio Aug 14 '18 at 11:11
  • Downvoter, please leave a comment, will be glad to fix or delete my answer. – Fabio Aug 14 '18 at 11:22
  • This change makes ClientEvaluationWarning – DiPix Aug 14 '18 at 11:26
  • @DiPix, materialize query before grouping. Put `.ToList()` after `.Where` clause. – Fabio Aug 14 '18 at 12:02
  • It's not helping. After that again is: `'An item with the same key has already been added. Key: 0'` – DiPix Aug 14 '18 at 12:05
  • @DiPix, add `.ToList()`, but do not remove `GroupBy`. `GroupBy` will remove duplicate `PolicyId` before creating a dictionary. – Fabio Aug 14 '18 at 12:33
  • @Fabio anyway your soulution override existing object. Actually I'd like to update because this `PolicySummaryDTO` might have other values already assigned. – DiPix Aug 14 '18 at 12:41
  • @Fabio check my Edit, I slightly refactored it. I used dictionary. Would be efficient now? It's shorter than your solution, and I think less complicated, you can suggest how I can improve it better if you know how, thanks :) – DiPix Aug 14 '18 at 13:32
  • Your solution is exactly what @Fabjan suggested in his answer. – Fabio Aug 14 '18 at 19:53