6
\$\begingroup\$

Take a look a this service method:

public static void CreateSocialEvent(int employeeId)
{
    using (var dbContext = new EmployeeEntities())
    {
        using (var transaction = dbContext.Database.BeginTransaction())
        {
            try
            {
                // I would prefer the next two statements to be in a separate method :
                var employee = dbContext.Employees.SingleOrDefault(e => e.IsActive 
                    && e.IsSocialEmployee);
                dbContext.SaveChanges();

                // I would prefer the next three statements to be in a separate method :
                var reminder = new Reminder 
                    {
                        EmployeeId = employee.EmployeeId,
                        CreationDate = DateTime.Now
                    }
                dbContext.Reminders.Add(reminder);
                dbContext.SaveChanges();

                // I would prefer the next three statements to be in a separate method :
                var socialMailingListSubscriber = new socialMailingListSubscriber
                    {
                        EmployeeId = employee.employeeId
                    }
                dbContext.SocialMailingListSubscribers.Add(socialMailingListSubscriber);
                dbContext.SaveChanges();

                // I would prefer the next three statements to be in a separate method :
                var subscription = new Subscription
                    {
                        subscription.EmployeeId = employee.Id
                    }
                dbContext.Subscriptions.Add(subscription);
                dbContext.SaveChanges();

                // I would prefer the next three statements to be in a separate method :
                var department = dbContext.Department.Find(employee.DepartmentId)
                    .Include(d => d.Subscribers);
                employee.Subscribers = department.Subscribers;
                dbContext.SaveChanges();

                transaction.Commit(); 
            }
            catch (Exception)
            {
                transaction.Rollback();
                throw;
            }
        }
    }
}

As you saw from my comments, I would prefer to segregate each operation in it's own method, to respect the single responsibility principle, as I don't like having very long methods that do many things.

Say I create a Reminder service that handles Reminders and a SocialMailingList service that handles MailingLists, I would have to inject either the dbContext or the transaction into these other services or into the other methods.

If I choose to inject the transaction, it won't work because if I open a new dbContext there, I cannot assign the injected transaction : it has to come from the same dbContext.

If I inject the dbContext entirely, I will have to write more code just to handle the dbContext lifetime. So, I'm close to implementing the Unit Of Work / Repository Pattern, and that's what I want to avoid at all costs.

If I understand correctly, DbContext in itself is a Unit Of Work, and the DbSet collections act as Repositories (am I right?). I don't like abstracting out something that has already been abstracted for me.

In other words, what should I do?

  1. Keep that long method there so I can keep the Entity-Framework code simple?
  2. Implement the Unit Of Work / Repository pattern so I can control the lifetime of my transaction?

I've never implemented the Unit Of Work / Repository Pattern so far. I've read a lot on that, though, and the amount of code to write to implement it just scares me!

I don't need to mock anything, I don't need to support any other rdbms than MS SQL Server, I don't even unit test anything in this project.

If it's better to implement it, can you suggest the simplest way to do so?

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! We actually encourage you to post your real code as is, rather than a simplified version, if it has any chance of being relevant to the review. \$\endgroup\$ – 200_success Nov 10 '14 at 1:17
  • \$\begingroup\$ Can you give an example of how injecting the dbContext will cause you to have to handle the lifetime? \$\endgroup\$ – jmoreno Nov 10 '14 at 3:17
  • \$\begingroup\$ The problem I have is more that all of my services will all depend of a DbContext, just for the sake of a transactional batch of operations. I use to instanciate-query-dispose of a DbContext for very short and specific operations. Injecting it throughout services seems like a code smell to me. \$\endgroup\$ – user57782 Nov 10 '14 at 3:30
1
\$\begingroup\$

What is the main purpose of the transaction? Is it to handle exceptions and then rollback? Or is there actually something to lock the database/table for everyone for during the creation of social media events. Can be that this is due to the demo code, but I see no need for the transaction itself.

If you remove the transaction, and just do SaveChanges once at the end, then you don't need the transaction. Which solves part of your problem I guess.

    public static void CreateSocialEvent(int employeeId)
    {
        using (var dbContext = new EmployeeEntities())
        {
            var employee = dbContext.Employees.FirstOrDefault(e => e.IsActive && e.IsSocialEmployee && e.EmployeeId == employeeId);
            if (employee == null)
            {
                throw new ArgumentException(string.Format("Unable to find employee by '{0}'", employeeId));
            }

            dbContext.Reminders.Add(new Reminder
            {
                EmployeeId = employee.EmployeeId,
                CreationDate = DateTime.Now
            });

            dbContext.SocialMailingListSubscribers.Add(new SocialMailingListSubscriber
            {
                EmployeeId = employee.EmployeeId
            });

            dbContext.Subscriptions.Add(new Subscription
            {
                EmployeeId = employee.EmployeeId
            });

            var department = dbContext.Departments
                .Include(d => d.Subscribers)
                .FirstOrDefault(d => d.DepartmentId == employee.DepartmentId);
            if (department == null)
            {
                throw new NullReferenceException("Departments could not be found");
            }
            employee.Subscribers = department.Subscribers;

            // Persist to database
            // if we fail to get here due to an exception, 
            // the using statement will dispose and the exception will be thrown
            dbContext.SaveChanges();
        }
    }

And you could move some of the creation of object parts, like in the simplest way:

public class Reminder
{
    public int EmployeeId { get; set; }
    public DateTime CreationDate { get; set; }

    public static Reminder CreateForEmployee(int employeeId)
    {
        return new Reminder
        {
            EmployeeId = employeeId,
            CreationDate = DateTime.Now
        };
    }

    public Reminder() { }

    public Reminder(int employeeId)
    {
        this.EmployeeId = employeeId;
        this.CreationDate = DateTime.Now;
    }
}

So you could do:

    public static void CreateSocialEvent(int employeeId)
    {
        using (var dbContext = new EmployeeEntities())
        {
            var employee = dbContext.Employees.FirstOrDefault(e => e.IsActive && e.IsSocialEmployee && e.EmployeeId == employeeId);

            if (employee == null)
            {
                throw new ArgumentException(string.Format("Unable to find employee by '{0}'", employeeId));
            }

            // with a static method
            dbContext.Reminders.Add(Reminder.CreateForEmployee(employeeId));

            // or with the constructor
            dbContext.Reminders.Add(new Reminder(employeeId));
        }
    }

To make it fit on one line, if that is what you want. Or use DI to move the responsibility of creating the object out of the method.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy