Code smell - LINQ to SQL Datacontext usage and more… - Refactoring work in progress

Following up the blog post about some major issues in the usage of the datacontext I dig into more details here.
Here is the initial blog post with an intro and the whole code

Lets start to look at the methods itself, starting with: "UpdateEmployeePositionHistory".

The name suggests that we are updating something with the values from the method.
Probably we are updating an entry in the database with ID:"EmployeeAssignmentID" and update that with the values: "EmployeeTypeID" and a new "startDate"

        public void UpdateEmployeePositionHistory(Guid EmployeeAssignmentID, Guid EmployeeTypeID, DateTime startDate)
        {
            var _positionAssignment = GetCurrentAssignment(EmployeeAssignmentID);
            var assignmentBefore = this.GetAssignStartDateBeforeEmployeePositionAssigment(_positionAssignment.EmployeeID, _positionAssignment.DateBegin);
            var assignmentAfter = this.GetAssignStartDateAfterEmployeePositionAssigment(_positionAssignment.EmployeeID, _positionAssignment.DateBegin);
            if (_positionAssignment != null)
            {
                _positionAssignment.EmployeeTypeID = EmployeeTypeID;
                _positionAssignment.DateBegin = startDate;
                if (assignmentBefore != null)
                {
                    assignmentBefore.DateEnd = startDate.AddDays(-1);
                }
                if (assignmentAfter == null)
                {
                    _positionAssignment.DateEnd = defaultMaxDate;
                }
                else
                {
                    _positionAssignment.DateEnd = assignmentAfter.DateBegin.AddDays(-1);
                }
            }
            db.SubmitChanges();
            this.ApplyPositionChangesToEmployee(_positionAssignment.EmployeeID);
            RuleManager.MarkEmployeeShiftAsDirty(_positionAssignment.EmployeeID, _positionAssignment.DateBegin, (DateTime)_positionAssignment.DateEnd);
        }

That is the case, but we are updating a previous entry as well. The method name didn't suggest that.

The first major problem that I see is the yellow:

If there is a current position (positionAssignment !=null highlighted yellow above) we are updating it.
If there is NO current position we are submitting changes to the database (although there are any), apply some position changes and mark those changes as dirty, so that our RuleProcessor can process them.

            db.SubmitChanges();
            this.ApplyPositionChangesToEmployee(_positionAssignment.EmployeeID);
            RuleManager.MarkEmployeeShiftAsDirty(_positionAssignment.EmployeeID, _positionAssignment.DateBegin, (DateTime)_positionAssignment.DateEnd);

This does not make sense, since no changes would not necessarily need to trigger a DB update and rule manager doesn't need to mark some entries as dirty.

 

Next problem:

We have 2 method calls in that method that have not a very good name I think:

   var assignmentBefore = this.GetAssignStartDateBeforeEmployeePositionAssigment(_positionAssignment.EmployeeID, _positionAssignment.DateBegin);
   var assignmentAfter = this.GetAssignStartDateAfterEmployeePositionAssigment(_positionAssignment.EmployeeID, _positionAssignment.DateBegin);

I will probably rename them to

   var assignmentBefore = this.GetAssignmentBeforeDate(_positionAssignment.EmployeeID, _positionAssignment.DateBegin);
   var assignmentAfter = this.GetAssignmentAfterDate(_positionAssignment.EmployeeID, _positionAssignment.DateBegin);

 

Long story short, I refactor this method without any tests around it. I know I shouldn't do that. We had tests that use a test database, but since my last time on this project all these tests are ignored and the test database is gone. Probably someone else couldn't be bothered.
This is just 1 of the pains with tests that use the database (Note: these are *not* unit tests).
What I could have done is mock the datacontext by following this blog post from Andrew Tokeley http://andrewtokeley.net/archive/2008/07/06/mocking-linq-to-sql-datacontext.aspx

Finally I changed the method from

        public void UpdateEmployeePositionHistory(Guid EmployeeAssignmentID, Guid EmployeeTypeID, DateTime startDate)
        {
            var _positionAssignment = GetCurrentAssignment(EmployeeAssignmentID);
            var assignmentBefore = this.GetAssignStartDateBeforeEmployeePositionAssigment(_positionAssignment.EmployeeID, _positionAssignment.DateBegin);
            var assignmentAfter = this.GetAssignStartDateAfterEmployeePositionAssigment(_positionAssignment.EmployeeID, _positionAssignment.DateBegin);
            if (_positionAssignment != null)
            {
                _positionAssignment.EmployeeTypeID = EmployeeTypeID;
                _positionAssignment.DateBegin = startDate;
                if (assignmentBefore != null)
                {
                    assignmentBefore.DateEnd = startDate.AddDays(-1);
                }
                if (assignmentAfter == null)
                {
                    _positionAssignment.DateEnd = defaultMaxDate;
                }
                else
                {
                    _positionAssignment.DateEnd = assignmentAfter.DateBegin.AddDays(-1);
                }
            }
            db.SubmitChanges();
            this.ApplyPositionChangesToEmployee(_positionAssignment.EmployeeID);
            RuleManager.MarkEmployeeShiftAsDirty(_positionAssignment.EmployeeID, _positionAssignment.DateBegin, (DateTime)_positionAssignment.DateEnd);
        }

 

to

        public void UpdateEmployeePositionHistory(Guid employeeAssignmentID, Guid employeeTypeID, DateTime startDate)
        {
            var assignment = UpdateAssignment(employeeAssignmentID, employeeTypeID, startDate);

            if (assignment != null)
            {
                FixUpdatedAssignment(assignment);
            }
        }

I don't like the null check in this method, but I live with it for now…

I extracted some code to "FixUpdatedAssignment", being the 3 method calls that we did before all the time.

        private void FixUpdatedAssignment(EmployeePositionHistoryAssignment assignment)
        {
            ApplyPositionChangesToEmployee(assignment.EmployeeID);
            DateTime endTime = assignment.DateEnd.HasValue ? assignment.DateEnd.Value : DefaultMaxDate;
            RuleManager.MarkEmployeeShiftAsDirty(assignment.EmployeeID, assignment.DateBegin, endTime);
        }

The other piece of code I fixed and extracted to "UpdateAssignment"

private EmployeePositionHistoryAssignment UpdateAssignment(Guid employeeAssignmentID, Guid employeeTypeID, DateTime startDate)
{
    using (NorthwindDataContext db = new NorthwindDataContext())
    {
        EmployeePositionHistoryAssignment assignment = GetAssignment(employeeAssignmentID);
        if (assignment != null)
        {
                    //Since we are updating the assignment before we have to get it from the current datacontext
                    var assignmentBefore = (from assignmentFromDB in db.EmployeePositionHistoryAssignments
                                    where
                                        assignmentFromDB.EmployeeID == assignment.EmployeeID &&
                                        assignmentFromDB.DateBegin < assignment.DateBegin
                                    orderby assignmentFromDB.DateBegin descending
                                    select assignmentFromDB).FirstOrDefault();
            
            var assignmentAfter = GetAssignmentAfterDate(assignment.EmployeeID,
                                                         assignment.DateBegin);
            assignment.EmployeeTypeID = employeeTypeID;
            assignment.DateBegin = startDate;
            if (assignmentBefore != null)
            {
                assignmentBefore.DateEnd = startDate.AddDays(-1);
            }
            if (assignmentAfter == null)
            {
                assignment.DateEnd = DefaultMaxDate;
            }
            else
            {
                assignment.DateEnd = assignmentAfter.DateBegin.AddDays(-1);
            }
        }
        db.SubmitChanges();
        return assignment;
    }
}

#1 The yellow above is *very* important. We have to get the "assignmentBefore" from the same datacontext. Otherwise the SubmitChanges wont update the "assignmentbefore". That was the reason we had a global datacontext object before as we figured out

#2 I removed the class-wide datacontext below and need to fix all other methods in this class

public class EmployeePositionAssignment
    {
        private NorthWindDataContext db = new NorthWindDataContext();
       // -- snip snip snip --

Now I am up to fix all the other methods that use the "global" datacontext… More fun coming….