2
\$\begingroup\$

I want to get opinions on the code I have put together for a centralized DataContext via a DataHelper class that I have created to be re-used on projects.

NOTE - there is a ton of code here, sorry about that, but I really wanted to layout out the complete approach and uses for my ideas. I'm an not saying this is the right approach, but it works for me so far (still playing with the approach, nothing in production yet, but very similar to stuff I have built over the years) and I really want to get constructive feedback from the community on what I have built to see if it is insane, great, can be improved, etc...

A few thoughts I put into this:

  1. Data Context needs to be stored in a common memory space, easily accessible
  2. Transactions should take the same approach
  3. It must be disposed of properly
  4. Allows for better separation of business logic for Saving and Deleting in transactions.

Here is the code for each item:

1 - First the data context stored in either the current HttpContext.Current.Items collection (so it only lives for the life of the page and only is fired up once at the first requested) or if the HttpContext doesn't exist uses a ThreadSlot (in which case that code most clean it up itself, like a console app using it...):

public static class DataHelper {
    /// <summary>
    /// Current Data Context object in the HTTPContext or Current Thread
    /// </summary>
    public static TemplateProjectContext Context {
        get {
            TemplateProjectContext context = null;

            if (HttpContext.Current == null) {
                LocalDataStoreSlot threadSlot = Thread.GetNamedDataSlot("DataHelper.CurrentContext");

                if (Thread.GetData(threadSlot) == null) {
                    context = new TemplateProjectContext();
                    Thread.SetData(threadSlot, context);
                } else {
                    context = (TemplateProjectContext)Thread.GetData(threadSlot);
                }
            } else {
                if (HttpContext.Current.Items["DataHelper.CurrentContext"] == null) {
                    context = new TemplateProjectContext();
                    HttpContext.Current.Items["DataHelper.CurrentContext"] = context;
                } else {
                    context = (TemplateProjectContext)HttpContext.Current.Items["DataHelper.CurrentContext"];
                }
            }

            return context;
        }
        set {
            if (HttpContext.Current == null) {
                if (value == null) {
                    Thread.FreeNamedDataSlot("DataHelper.CurrentContext");
                } else {
                    LocalDataStoreSlot threadSlot = Thread.GetNamedDataSlot("DataHelper.CurrentContext");
                    Thread.SetData(threadSlot, value);
                }
            } else {
                if (value == null)
                    HttpContext.Current.Items.Remove("DataHelper.CurrentContext");
                else
                    HttpContext.Current.Items["DataHelper.CurrentContext"] = value;
            }
        }
    }
...

2 - To support transactions, I use a similar approach, and also include helper methods to Begin, Commit and Rollback:

    /// <summary>
    /// Current Transaction object in the HTTPContext or Current Thread
    /// </summary>
    public static DbTransaction Transaction {
        get {
            if (HttpContext.Current == null) {
                LocalDataStoreSlot threadSlot = Thread.GetNamedDataSlot("currentTransaction");

                if (Thread.GetData(threadSlot) == null) {
                    return null;
                } else {
                    return (DbTransaction)Thread.GetData(threadSlot);
                }
            } else {
                if (HttpContext.Current.Items["currentTransaction"] == null) {
                    return null;
                } else {
                    return (DbTransaction)HttpContext.Current.Items["currentTransaction"];
                }
            }
        }
        set {
            if (HttpContext.Current == null) {
                LocalDataStoreSlot threadSlot = Thread.GetNamedDataSlot("currentTransaction");
                Thread.SetData(threadSlot, value);
            } else {
                HttpContext.Current.Items["currentTransaction"] = value;
            }
        }
    }

    /// <summary>
    /// Begins a transaction based on the common connection and transaction
    /// </summary>
    public static void BeginTransaction() {
        DataHelper.Transaction = DataHelper.CreateSqlTransaction();
    }
    /// <summary>
    /// Creates a SqlTransaction object based on the current common connection
    /// </summary>
    /// <returns>A new SqlTransaction object for the current common connection</returns>
    public static DbTransaction CreateSqlTransaction() {
        return CreateSqlTransaction(DataHelper.Context.Connection);
    }
    /// <summary>
    /// Creates a SqlTransaction object for the requested connection object
    /// </summary>
    /// <param name="connection">Reference to the connection object the transaction should be created for</param>
    /// <returns>New transaction object for the requested connection</returns>
    public static DbTransaction CreateSqlTransaction(DbConnection connection) {
        if (connection.State != ConnectionState.Open) connection.Open();
        return connection.BeginTransaction();
    }
    /// <summary>
    /// Rolls back and cleans up the current common transaction
    /// </summary>
    public static void RollbackTransaction() {
        if (DataHelper.Transaction != null) {
            DataHelper.RollbackTransaction(DataHelper.Transaction);

            if (HttpContext.Current == null) {
                Thread.FreeNamedDataSlot("currentTransaction");
            } else {
                HttpContext.Current.Items.Remove("currentTransaction");
            }
        }
    }
    /// <summary>
    /// Rolls back and disposes of the requested transaction
    /// </summary>
    /// <param name="transaction">The transaction to rollback</param>
    public static void RollbackTransaction(DbTransaction transaction) {
        transaction.Rollback();
        transaction.Dispose();
    }

    /// <summary>
    /// Commits and cleans up the current common transaction
    /// </summary>
    public static void CommitTransaction() {
        if (DataHelper.Transaction != null) {
            DataHelper.CommitTransaction(DataHelper.Transaction);

            if (HttpContext.Current == null) {
                Thread.FreeNamedDataSlot("currentTransaction");
            } else {
                HttpContext.Current.Items.Remove("currentTransaction");
            }
        }
    }
    /// <summary>
    /// Commits and disposes of the requested transaction
    /// </summary>
    /// <param name="transaction">The transaction to commit</param>
    public static void CommitTransaction(DbTransaction transaction) {
        transaction.Commit();
        transaction.Dispose();
    }

3 - Clean and easy Disposal

    /// <summary>
    /// Cleans up the currently active connection
    /// </summary>
    public static void Dispose() {
        if (HttpContext.Current == null) {
            LocalDataStoreSlot threadSlot = Thread.GetNamedDataSlot("DataHelper.CurrentContext");
            if (Thread.GetData(threadSlot) != null) {
                DbTransaction transaction = DataHelper.Transaction;
                if (transaction != null) {
                    DataHelper.CommitTransaction(transaction);
                    Thread.FreeNamedDataSlot("currentTransaction");
                }

                ((TemplateProjectContext)Thread.GetData(threadSlot)).Dispose();
                Thread.FreeNamedDataSlot("DataHelper.CurrentContext");
            }
        } else {
            if (HttpContext.Current.Items["DataHelper.CurrentContext"] != null) {
                DbTransaction transaction = DataHelper.Transaction;
                if (transaction != null) {
                    DataHelper.CommitTransaction(transaction);
                    HttpContext.Current.Items.Remove("currentTransaction");
                }

                ((TemplateProjectContext)HttpContext.Current.Items["DataHelper.CurrentContext"]).Dispose();
                HttpContext.Current.Items.Remove("DataHelper.CurrentContext");
            }
        }
    }

3b - I'm building this in MVC, so I have a "base" Controller class that all my controllers inherit from - this way the Context only lives from when first accessed on a request, and until the page is disposed, that way its not too "long running"

using System.Web.Mvc;
using Core.ClassLibrary;
using TemplateProject.Business;
using TemplateProject.ClassLibrary;

namespace TemplateProject.Web.Mvc {
    public class SiteController : Controller {
        protected override void Dispose(bool disposing) {
            DataHelper.Dispose();
            base.Dispose(disposing);
        }
    }
}

4 - So I am big on business classes, separation of concerns, reusable code, all that wonderful stuff. I have an approach that I call "Entity Generic" that can be applied to any entity in my system - for example, Addresses and Phones

A Customer can have 1 or more of each, along with a Store, Person, or anything really - so why add street, city, state, etc to every thing that needs it when you can just build an Address entity, that takes a Foreign Type and Key (what I call EntityType and EntityId) - then you have a re-usable business object, supporting UI control, etc - so you build it once and re-use it everywhere.

This is where the centralized approach I am pushing for here really comes in handy and I think makes the code much cleaner than having to pass the current data context/transaction into every method.

Take for example that you have a Page for a customer, the Model includes the Customer data, Contact, Address and a few Phone Numbers (main, fax, or cell, whatever)

When getting a Customer Edit Model for the page, here is a bit of the code I have put together (see how I use the DataHelper.Context in the LINQ):

    public static CustomerEditModel FetchEditModel(int customerId) {
        if (customerId == 0) {
            CustomerEditModel model = new CustomerEditModel();
            model.MainContact = new CustomerContactEditModel();

            model.MainAddress = new AddressEditModel();
            model.ShippingAddress = new AddressEditModel();

            model.Phone = new PhoneEditModel();
            model.Cell = new PhoneEditModel();
            model.Fax = new PhoneEditModel();

            return model;
        } else {
            var output = (from c in DataHelper.Context.Customers
                          where c.CustomerId == customerId
                          select new CustomerEditModel {
                              CustomerId = c.CustomerId,
                              CompanyName = c.CompanyName
                          }).SingleOrDefault();

            if (output != null) {
                output.MainContact = CustomerContact.FetchEditModelByPrimary(customerId) ?? new CustomerContactEditModel();

                output.MainAddress = Address.FetchEditModelByType(BusinessEntityTypes.Customer, customerId, AddressTypes.Main) ?? new AddressEditModel();
                output.ShippingAddress = Address.FetchEditModelByType(BusinessEntityTypes.Customer, customerId, AddressTypes.Shipping) ?? new AddressEditModel();

                output.Phone = Phone.FetchEditModelByType(BusinessEntityTypes.Customer, customerId, PhoneTypes.Main) ?? new PhoneEditModel();
                output.Cell = Phone.FetchEditModelByType(BusinessEntityTypes.Customer, customerId, PhoneTypes.Cell) ?? new PhoneEditModel();
                output.Fax = Phone.FetchEditModelByType(BusinessEntityTypes.Customer, customerId, PhoneTypes.Fax) ?? new PhoneEditModel();
            }

            return output;
        }
    }

And here is a sample of the the phone returning the Edit model to be used:

    public static PhoneEditModel FetchEditModelByType(byte entityType, int entityId, byte phoneType) {
        return (from p in DataHelper.Context.Phones
                where p.EntityType == entityType
                    && p.EntityId == entityId
                    && p.PhoneType == phoneType
                select new PhoneEditModel {
                    PhoneId = p.PhoneId,
                    PhoneNumber = p.PhoneNumber,
                    Extension = p.Extension
                }).FirstOrDefault();
    }

Now the page has posted back and this all needs to be save, so the Save method in my control just lets the business object handle this all:

    [Authorize(Roles = SiteRoles.SiteAdministrator + ", " + SiteRoles.Customers_Edit)]
    [HttpPost]
    public ActionResult Create(CustomerEditModel customer) {
        return CreateOrEdit(customer);
    }
    [Authorize(Roles = SiteRoles.SiteAdministrator + ", " + SiteRoles.Customers_Edit)]
    [HttpPost]
    public ActionResult Edit(CustomerEditModel customer) {
        return CreateOrEdit(customer);
    }
    private ActionResult CreateOrEdit(CustomerEditModel customer) {
        if (ModelState.IsValid) {
            SaveResult result = Customer.SaveEditModel(customer);
            if (result.Success) {
                return RedirectToAction("Index");
            } else {
                foreach (KeyValuePair<string, string> error in result.ErrorMessages) ModelState.AddModelError(error.Key, error.Value);
            }
        }

        return View(customer);
    }

And inside the Customer business object - it handles the transaction centrally and lets the Contact, Address and Phone business classes do their thing and really not worry about the transaction:

    public static SaveResult SaveEditModel(CustomerEditModel model) {
        SaveResult output = new SaveResult();

        DataHelper.BeginTransaction();
        try {
            Customer customer = null;
            if (model.CustomerId == 0) customer = new Customer();
            else customer = DataHelper.Context.Customers.Single(c => c.CustomerId == model.CustomerId);

            if (customer == null) {
                output.Success = false;
                output.ErrorMessages.Add("CustomerNotFound", "Unable to find the requested Customer record to update");
            } else {
                customer.CompanyName = model.CompanyName;

                if (model.CustomerId == 0) {
                    customer.SiteGroup = CoreSession.CoreSettings.CurrentSiteGroup;
                    customer.CreatedDate = DateTime.Now;
                    customer.CreatedBy = SiteLogin.Session.ActiveUser;
                    DataHelper.Context.Customers.AddObject(customer);
                } else {
                    customer.ModifiedDate = DateTime.Now;
                    customer.ModifiedBy = SiteLogin.Session.ActiveUser;
                }
                DataHelper.Context.SaveChanges();

                SaveResult result = Address.SaveEditModel(model.MainAddress, BusinessEntityTypes.Customer, customer.CustomerId, AddressTypes.Main, false);
                if (!result.Success) {
                    output.Success = false;
                    output.ErrorMessages.Concat(result.ErrorMessages);
                }

                result = Address.SaveEditModel(model.ShippingAddress, BusinessEntityTypes.Customer, customer.CustomerId, AddressTypes.Shipping, false);
                if (!result.Success) {
                    output.Success = false;
                    output.ErrorMessages.Concat(result.ErrorMessages);
                }

                result = Phone.SaveEditModel(model.Phone, BusinessEntityTypes.Customer, customer.CustomerId, PhoneTypes.Main, false);
                if (!result.Success) {
                    output.Success = false;
                    output.ErrorMessages.Concat(result.ErrorMessages);
                }

                result = Phone.SaveEditModel(model.Fax, BusinessEntityTypes.Customer, customer.CustomerId, PhoneTypes.Fax, false);
                if (!result.Success) {
                    output.Success = false;
                    output.ErrorMessages.Concat(result.ErrorMessages);
                }

                result = Phone.SaveEditModel(model.Cell, BusinessEntityTypes.Customer, customer.CustomerId, PhoneTypes.Cell, false);
                if (!result.Success) {
                    output.Success = false;
                    output.ErrorMessages.Concat(result.ErrorMessages);
                }

                result = CustomerContact.SaveEditModel(model.MainContact, customer.CustomerId, false);
                if (!result.Success) {
                    output.Success = false;
                    output.ErrorMessages.Concat(result.ErrorMessages);
                }

                if (output.Success) {
                    DataHelper.Context.SaveChanges();
                    DataHelper.CommitTransaction();
                } else {
                    DataHelper.RollbackTransaction();
                }
            }
        } catch (Exception exp) {
            DataHelper.RollbackTransaction();

            ErrorHandler.Handle(exp, true);

            output.Success = false;
            output.ErrorMessages.Add(exp.GetType().ToString(), exp.Message);
            output.Exceptions.Add(exp);
        }

        return output;
    }

Notice how each Address, Phone, Etc is handled by its own business class, here is the Phone's save method - notice how it doesn't actually do the save here unless you tell it to (save is handled in the Customer's method so save is called just once for the context)

    public static SaveResult SaveEditModel(PhoneEditModel model, byte entityType, int entityId, byte phoneType, bool saveChanges) {
        SaveResult output = new SaveResult();

        try {
            if (model != null) {
                Phone phone = null;
                if (model.PhoneId != 0) {
                    phone = DataHelper.Context.Phones.Single(x => x.PhoneId == model.PhoneId);
                    if (phone == null) {
                        output.Success = false;
                        output.ErrorMessages.Add("PhoneNotFound", "Unable to find the requested Phone record to update");
                    }
                }

                if (string.IsNullOrEmpty(model.PhoneNumber)) {
                    if (model.PhoneId != 0 && phone != null) {
                        DataHelper.Context.Phones.DeleteObject(phone);
                        if (saveChanges) DataHelper.Context.SaveChanges();
                    }
                } else {
                    if (model.PhoneId == 0) phone = new Phone();
                    if (phone != null) {
                        phone.EntityType = entityType;
                        phone.EntityId = entityId;
                        phone.PhoneType = phoneType;

                        phone.PhoneNumber = model.PhoneNumber;
                        phone.Extension = model.Extension;

                        if (model.PhoneId == 0) {
                            phone.CreatedDate = DateTime.Now;
                            phone.CreatedBy = SiteLogin.Session.ActiveUser;
                            DataHelper.Context.Phones.AddObject(phone);
                        } else {
                            phone.ModifiedDate = DateTime.Now;
                            phone.ModifiedBy = SiteLogin.Session.ActiveUser;
                        }

                        if (saveChanges) DataHelper.Context.SaveChanges();
                    }
                }
            }
        } catch (Exception exp) {
            ErrorHandler.Handle(exp, true);

            output.Success = false;
            output.ErrorMessages.Add(exp.GetType().ToString(), exp.Message);
            output.Exceptions.Add(exp);
        }

        return output;
    }

FYI - SaveResult is just a little container class that I used to get detailed information back if a save fails:

public class SaveResult {
    private bool _success = true;
    public bool Success {
        get { return _success; }
        set { _success = value; }
    }

    private Dictionary<string, string> _errorMessages = new Dictionary<string, string>();
    public Dictionary<string, string> ErrorMessages {
        get { return _errorMessages; }
        set { _errorMessages = value; }
    }

    private List<Exception> _exceptions = new List<Exception>();
    public List<Exception> Exceptions {
        get { return _exceptions; }
        set { _exceptions = value; }
    }
}

The other piece to this is the re-usable UI code for the Phone, Address, etc - which handles all the validation, etc in just one location too.

So, let your thoughts flow and thanks for taking the time to read/review this huge post!

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

I don't mean to hurt your feelings, because you've obviously put a lot of work into this, but I personally don't like it. It seems like you've forsaken Inversion of Control and Dependency Injection for a collection of static "God" classes and methods.

For example, the Context property of your DataHelper method. Pretty much any decent IoC container can do this for you in a non-static way. And you have a static Dispose method? Usually that method name is reserved for non-static classes so that instances are automatically cleaned up inside of a using statement (classes that implement the IDisposable interface).

You also have persistence logic in your viewmodels / editmodels. Persistence is an infrastructure concern, whereas these MVC models are HTTP/web concerns. If you should be doing persistence anywhere in MVC, it should be in the controller or an ActionFilter (but ideally you should outsource persistence to an interface).

Have a read of these two articles. Try out their tactics. I think you will find a better separation of concerns between infrastructure, application, and presentation layers.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ No problem on the feelings - I posted this to get feedback like this, I appreciate good feedback and direction, that's really what I want here is to open the discussion. I will read through the articles you posted. One of the reasons I did this approach was because the "save" method in each class handles itself, and with the "entity generic" classes needed to have the FK defined up front, had to save off the parent class first - then call the child save methods with the new Id... my thinking has change to make the database use many-to-many tables for the child entities..... \$\endgroup\$ Jul 9, 2012 at 15:43
  • \$\begingroup\$ That removes the need for a "manual" transaction and eliminates the Data Helper. I am not sure I've forsaken DI or IoC - I don't use them since all my tests actually just hit the database (they actually build up the data for themselves as they test - just requires a clean DB for each test). As for the persistence logic note - I'm afraid I don't understand and probably need more discussion on that - The models I have are purely a structure for passing data between view/business classes (handled by the controller) - i'm a neophyte with MVC so could be I am understanding the use wrong? \$\endgroup\$ Jul 9, 2012 at 15:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.