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

image
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. image44 The method name didn't suggest that.

The first major problem that I see is the yellow:

Warning Problem ahead 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:

image4 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….

4 comments:

Yngve B. Nilsen said...

I would also say that the null-check is in the completely wrong place... The following lines (line 3, 4 in the method)

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

will crash if _positionAssignment == null.

Peter Gfader said...

Hi Yngve

You are right!
In the code before the refactoring, the accessof "_positionAssignment.EmployeeID" would throw NullReferenceException...

It was worse than I thought ;-)

Nick said...

Nice! post its really informative....

Software Product Development Services: - Ampere offers full cycle custom software programming services, from product idea, offshore software development to outsourcing support and enhancement.

Sim Cura said...

This looks absolutely perfect. All these tinny details are made with lot of background information and inspiration, both of which we all need, thanks for providing such helpful information here.

Post a Comment

Latest Posts

Popular Posts