1

I have two tables, Clients and Physicians. Each Client has a list of primary keys for their physicians (PhysicianIds), which is flattened to a comma-delimited string for database storage (PhysicianStore).

One of my action methods requires finding which client a physician belongs to, but I can't find a way to do this without evaluating the entire table with .ToList(). The below code doesn't work because .Split() doesn't work with LINQ-to-Entities.

Do I need to add a foreign key to Physician?

// Data model / DTO
public class ClientModel
{
    public List<int> PhysicianIds { get; set; }
    public string PhysicianStore
    {
        get { return string.Join(",", PhysicianIds.ConvertAll<string>(i => i.ToString())); }
        set { PhysicianIds = value.Split(',').Select(str => int.Parse(str)).ToList(); }
        //set { PhysicianIds = value.Split(',').ToList().ConvertAll<int>(str => int.Parse(str)); }
    }
}

public class PhysiciansController
{   
    // Disposed in full code
    private MyDbContext db = new MyDbContext();

    public async Task<ActionResult> Details(int? id)
    {
        if (id == null)
        {
            return HttpError(HttpStatusCode.BadRequest);
        }
        PhysicianModel pm = await db.Physicians.FindAsync(id);
        if (pm == null)
        {
            return HttpError(HttpStatusCode.NotFound);
        }
        // Have to use PhysicianStore here because PhysicianIds is not a column in the DB
        return View(new PhysicianDetailsViewModel(pm, db.Clients.Where(c => c.PhysicianStore.Split(',').Contains(pm.Id.ToString()))
                                                                                                       .FirstOrDefault()?.Name));
    }
}

Edit: Per comments, I really should be using navigation properties. I'll pursue this and ask another question if needed.

Sinjai
  • 1,085
  • 1
  • 15
  • 34
  • [Is storing a delimited list in a database column really that bad?](https://stackoverflow.com/questions/3653462/is-storing-a-delimited-list-in-a-database-column-really-that-bad) –  Oct 26 '17 at 05:52
  • @Stephen Good point, but I don't see a solution in those answers. – Sinjai Oct 26 '17 at 06:02
  • I was just pointing out how bad it is to have a comma-delimited string for database storage :) –  Oct 26 '17 at 06:03
  • @Stephen I agree, but I don't know what else to do. – Sinjai Oct 26 '17 at 06:03
  • You should follow an EntityFramework tutorial, just search in Google One-To-Many EntityFramework. You will come up with navigation properties that should help you with your problem. – dbraillon Oct 26 '17 at 06:17
  • Ignoring the table design, what's the actual issue with your code - `string.Split` not supported in LINQ to Entities? – Ivan Stoev Oct 26 '17 at 07:11
  • I don't really see the one-to-many relation from a logical point of view. Wouldn't a physician have multiple clients, making the whole thing a many-to-many? – grek40 Oct 26 '17 at 07:11
  • @IvanStoev Crap, forgot that. Yes, that's the issue. – Sinjai Oct 26 '17 at 07:13
  • 1
    @grek40 Each client has several physicians. Physicians aren't at several practices, which is what a client is here. – Sinjai Oct 26 '17 at 07:14
  • Ok, that makes sense (ignoring the 2-job-physicians). Can you add your Physician model class? – grek40 Oct 26 '17 at 07:16
  • @grek40 I could do that after I sleep (2:30 AM here), but I'm not sure why it's relevant. – Sinjai Oct 26 '17 at 07:20
  • Basically, it is relevant because with the "right" design of Physician, retrieving the associated client should be completely trivial. So there is something wrong with your current design, else you would never ask the question. – grek40 Oct 26 '17 at 07:39
  • @Sinjai So if `physicians aren't at several practices`, why not simply give your `Physician` a `ClientId`, instead of trying to push many `PhysicianIds` on your `Client`? This is how you should be handling it for a one-to-many relationship, **every item in the "many" should have a single reference to an item in the "one"**. Many physicians are able to refer to the same `ClientId`, but every physician will only have one `ClientId`. – Flater Oct 26 '17 at 08:05
  • @Flater I guess that didn't/doesn't make sense to me logistically. Then the only way to list a client's physicians is by searching the entire `Physicians` table, no? – Sinjai Oct 27 '17 at 01:00
  • @grek40 There's nothing worthwhile to see there because `Physician` has no foreign key, like the question says. See the above comments between Flater/myself for that. – Sinjai Oct 27 '17 at 01:16
  • Ok, then the answer is *"Add a foreign key to the Physician entity and remove the Id list from client."*. Is that enough for you? – grek40 Oct 27 '17 at 05:32
  • @grek40 Not sure how to manage that, which is the only reason the relationship is one-way right now. I assume the answer to that is "navigation properties do it automagically". – Sinjai Oct 27 '17 at 05:35
  • Well *\*surprise\** you have to implement some changes in the `Physicians` class and I don't see why I should invent such a class for an answer if you *for whatever reason* decide it's not going into the question. – grek40 Oct 27 '17 at 06:13
  • 1
    @grek40 I was going to ask a new question because the problem of the question changed when the suggestion moved to "use navigation properties". As always, you're under no obligation to answer. – Sinjai Oct 27 '17 at 15:06
  • @Sinjai `SELECT * FROM Physicians WHERE ClientId = 123` You don't need to list the whole table. Searching a table is not an expensive operation, PK/FK fields are indexed and optimized for quick searching. Besides, even if you were to get a comma separated list of physician IDs from your client, you would **still** have to query the `Physicians` table to retrieve the physicians, so your point is somewhat moot :) – Flater Oct 28 '17 at 08:38
  • @Flater Very good point. Someone else said both of them should have a foreign key though. – Sinjai Oct 28 '17 at 15:30

1 Answers1

2

Ignoring the database design, the main issue as I understand is that LINQ to Entities does not support string.Split. But string.Concat, string.Contains and ToString are supported, so you can use the following trick:

var token = "," + pm.Id.ToString() + ",";

var query = db.Clients
    .Where(c => ("," + c.PhysicianStore + ",").Contains(token));

The trick is to enclose with "," both the search and target terms. This way it handles correctly the start, end and middle elements of the list, and will not produced false positives when searching for let say "1" inside "12,21".

Ivan Stoev
  • 195,425
  • 15
  • 312
  • 343
  • Clever bandaid, thanks. Quick question that doesn't warrant its own post: Is either of the two setters of `PhysicianStore` superior? There's a 99% chance it will be getting removed anyway, but for the sake of my education... – Sinjai Oct 27 '17 at 01:14
  • It really doesn't matter. I personally prefer the variants with general `Select` for both getter and setter rather than `List` specific methods like `ConvertAll`. – Ivan Stoev Oct 27 '17 at 06:47