3
\$\begingroup\$

I'm using Entity Framework 6 with database first. See the image for the relevant database schemaenter image description here

I have the following method to save a Set to the database:

/// <summary>
/// Saves the list of data sources. This assumes that the ModelTemplateID, SetNumber and TaskNumber
/// are already properly set. Adds a new record to the DataSourceGroups table if necessary.
/// </summary>
/// <param name="dataSources">The list of data sources to save</param>
public void SaveSet(IEnumerable<DataSource> dataSources)
{
    using (var db = _contextFactory.GetContext())
    {
        foreach (var dataSource in dataSources)
        {
            var dataSourceFromDb =
                db.DataSources.FirstOrDefault(x => x.DataSourceID == dataSource.DataSourceID);

            if (dataSourceFromDb == null)
            { // not in database yet, add it
                var dataSourceGroupFromDb =
                    db.DataSourceGroups.SingleOrDefault(
                        x => x.ModelTemplateID == dataSource.ModelTemplateID &&
                             x.SetNumber == dataSource.SetNumber &&
                             x.TaskNumber == dataSource.TaskNumber);
                if (dataSourceGroupFromDb == null)
                { // Add new DataSourceGroup to db
                    db.DataSourceGroups.Add(new DataSourceGroup
                    {
                        ModelTemplateID = dataSource.ModelTemplateID,
                        SetNumber = dataSource.SetNumber,
                        TaskNumber = dataSource.TaskNumber
                    });
                }
                db.DataSources.Add(dataSource);
            }
            else
            { // already in database, if any fields are different then update it
                if (dataSourceFromDb.QueryID      != dataSource.QueryID || // todo make DataSource Comparer
                    dataSourceFromDb.SourceType   != dataSource.SourceType ||
                    dataSourceFromDb.TargetFormat != dataSource.TargetFormat ||
                    dataSourceFromDb.SourcePath   != dataSource.SourcePath ||
                    dataSourceFromDb.TargetPath   != dataSource.TargetPath)
                {
                    db.DataSources.Attach(dataSource);
                    db.Entry(dataSource).State = EntityState.Modified;
                }
            }
        }
        db.SaveChanges();
    }
}

Is this the best way to implement this method? Should I be attaching the DataSources to the Set when they are created and then just pass the Set into the method? Any other critiques welcome.

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$
/// <summary>
/// Saves the list of data sources. This assumes that the ModelTemplateID, SetNumber and TaskNumber
/// are already properly set. Adds a new record to the DataSourceGroups table if necessary.
/// </summary>
/// <param name="dataSources">The list of data sources to save</param>
public void SaveSet(IEnumerable<DataSource> dataSources)

The method name is telling a lie. SaveSet would be appropriate if the method took an IEnumerable<Set> sets parameter. I would have called the method SaveDataSources.

Per the XML comments, the method is making assumptions, but it does nothing to verify these assumptions, and since the method has no return type, the only way you can fail is by letting it throw - throwing is good in this situation, however it's best to fail early, if possible before you even create the context instance.

This is done with a guard clause at the top of your method, that aborts the call if an argument is invalid - for example:

if (dataSources.Any(dataSource => 
       dataSource.ModelTemplateID == default(typeof(dataSource.ModelTemplateID))
    || dataSource.SetNumber == default(typeof(dataSource.ModelTemplateID))
    || dataSource.TaskNumber == default(typeof(dataSource.TaskNumber))))
{
    throw new ArgumentException("Members 'ModelTemplateID', 'SetNumber' and 'TaskNumber' not set.", "dataSources");
}

This makes your code throw a more meaningful exception, instead of adding half the items, throwing some foreign key exception and then disposing the context without persisting anything.


using (var db = _contextFactory.GetContext())

While a conveniently short identifier, db is a misleading name to use for a DbContext; it's more of a context than a database, so context would be more appropriate.

I like that the method isn't directly responsible for creating the DbContext instance, however GetContext() sounds like you're not creating anything, rather just returning an existing instance - which is counter-intuitive in the context (pun not intended) of a factory. CreateContext would be better, and since _contextFactory is already a quite descriptive name, a Create method wouldn't be any less clear:

using (var context = _contextFactory.Create())
{

foreach (var dataSource in dataSources)
{
    var dataSourceFromDb =
        db.DataSources.FirstOrDefault(x => x.DataSourceID == dataSource.DataSourceID);

Unless I look at your schema and see that DataSourceID is the entity's primary key, I have no way of being sure that you're expecting only a single record, because FirstOrDefault could just as well operate on 20 entities and only return the first that matches the criteria.

This would be less ambiguous, because if more than 1 entity is returned, SingleOrDefault would throw an exception:

foreach (var dataSource in dataSources)
{
    var dataSourceFromDb = 
            context.DataSources
                   .SingleOrDefault(e => e.DataSourceID == dataSource.DataSourceID);

This part looks like it belongs in another method:

        if (dataSourceFromDb == null)
        { // not in database yet, add it
            var dataSourceGroupFromDb =
                db.DataSourceGroups.SingleOrDefault(
                    x => x.ModelTemplateID == dataSource.ModelTemplateID &&
                         x.SetNumber == dataSource.SetNumber &&
                         x.TaskNumber == dataSource.TaskNumber);
            if (dataSourceGroupFromDb == null)
            { // Add new DataSourceGroup to db
                db.DataSourceGroups.Add(new DataSourceGroup
                {
                    ModelTemplateID = dataSource.ModelTemplateID,
                    SetNumber = dataSource.SetNumber,
                    TaskNumber = dataSource.TaskNumber
                });
            }
            db.DataSources.Add(dataSource);

I don't like the comments on the same line as the scope-opening brace { //, and these comments wouldn't be needed at all if you extracted the method; the added level of abstraction would speak for itself:

if (dataSourceFromDb == null)
{
    EnsureDataSourceGroupExists(context, dataSource);
    context.DataSources.Add(dataSource)
}
else
{
    dataSourceFromDb.QueryID = dataSource.QueryID;
    dataSourceFromDb.SourceType = dataSource.SourceType;
    dataSourceFromDb.TargetFormat = dataSource.TargetFormat;
    dataSourceFromDb.SourcePath = dataSource.SourcePath;
    dataSourceFromDb.TargetPath = dataSource.TargetPath;
}

context.SaveChanges();

Notice the how the update code is simply copying the values from dataSource into the members of dataSourceFromDb - that entity is already attached and managed by the change tracker. Why complicate things? It's there, might as well use it!

One thing your code also assumes, is that SaveChanges will insert the DataSourceGroup entity before it inserts dataSource. I don't know about EF6, but in other versions one cannot make that assumption, so EnsureDataSourceGroupExists would call SaveChanges on the context if a new entity was created. Not doing this is like asking for a foreign key violation.

\$\endgroup\$
2
  • \$\begingroup\$ We'll have to agree to disagree on the name of the method; in our domain a Set is a list of DataSources. Based on code elsewhere in the application, I don't think that I need to check that the ModelTemplateID, SetNumber and TaskNumber are not set, they are guaranteed to be set when the objects are created. Good suggestion on changing the name of GetContext() to Create(); it is returning a new instance each time. Also a good suggestion on changing FirstOrDefault() to SingleOrDefault(). \$\endgroup\$ Jun 13, 2014 at 15:09
  • \$\begingroup\$ I also agree that the method should be split up some more. Good suggestion on just copying the properties, that's due to my ignorance of EF. I haven't run into any FK issues yet, but I'll be sure to call SaveChanges when inserting a DataSourceGroup to make sure. Thanks a lot. \$\endgroup\$ Jun 13, 2014 at 15:10

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.