Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm still trying to learn how to write good tests (and write testable code).

This is the function I want to verify,

 void IAutoTisDal.UpdateRange(Range range)
  {
     using (var repo = _repositoryFactory.GetRepository(_baseline))
     {
        var rangeqry = from r in repo.Query<Range>()
                       where r.Id == range.Id
                       select r;
        var rangeToUpdate = rangeqry.SingleOrDefault();
        if (rangeToUpdate == null)
           throw new Exception(string.Format("Range ID {0} not found", range.Id));

        rangeToUpdate.Status = range.Status;
        rangeToUpdate.FirstPsn = range.FirstPsn;
        rangeToUpdate.LastPsn = range.LastPsn;
        rangeToUpdate.PageCount = (int)(range.LastPsn - range.FirstPsn);
        rangeToUpdate.CourierId = range.CourierId;
        rangeToUpdate.WorkflowServer = range.WorkflowServer;
        rangeToUpdate.DoubleFeedInd = range.DoubleFeedInd;

        repo.Commit();
     }
  }

And the unit test:

  [TestMethod]
  public void UpdateRange_RangesPropertiesAreCopied()
  {
     var rangeToUpdate = new Range()
     {
        Id = 1, CourierId = 0, Status = 20,
        WorkflowServer = null, 
        FirstPsn = -1, LastPsn = -1, PageCount = 0
     };
     var rangeToSave = new Range()
     {
        Id = 1, CourierId = 1234, Status = 40,
        WorkflowServer = new PhysicalStation(){ Id = 1, Name = "fakeserver"},
        FirstPsn = 123, LastPsn = 456, PageCount = 333
     };
     var repoMock = new Mock<IRepository>();
     repoMock.Setup(x => x.Query<Range>()).Returns(new List<Range>(new[] {rangeToUpdate}).AsQueryable()).Verifiable();
     repoMock.Setup(x => x.Commit()).Verifiable();
     var factoryMock = new Mock<IRepositoryFactory>();
     factoryMock.Setup(x => x.GetRepository(It.IsAny<string>())).Returns(repoMock.Object);
     IAutoTisDal dal = new AutoTisDal(factoryMock.Object, "");

     dal.UpdateRange(rangeToSave);

     repoMock.VerifyAll();

     Assert.AreEqual(rangeToUpdate.Id, rangeToSave.Id);
     Assert.AreEqual(rangeToUpdate.CourierId, rangeToSave.CourierId);
     Assert.AreEqual(rangeToUpdate.LastPsn, rangeToSave.LastPsn);
     Assert.AreEqual(rangeToUpdate.FirstPsn, rangeToSave.FirstPsn);
     Assert.AreEqual(rangeToUpdate.PageCount, rangeToSave.LastPsn - rangeToSave.FirstPsn);
     Assert.AreSame(rangeToUpdate.WorkflowServer, rangeToSave.WorkflowServer);
     Assert.AreEqual(rangeToUpdate.Status, rangeToSave.Status);
  }

I know I need to write another test against the expected exception if no RangeId matches.

I also realize I'm writing test after the code.

share|improve this question
    
side topic: Would IAutoTisDal be a considered service layer or a data access layer? This is a problem-domain-specific interface that calls my IRepository. The concrete IRepository uses NHibernate to exposes Query<T>() and Commit(). –  hometoast Nov 13 '12 at 14:40
add comment

2 Answers 2

up vote 4 down vote accepted

First of all, your single method is doing multiple things:

  1. Get Range by id
  2. Copy properties from one instance to another
  3. Commit

Let's try to separate responsibilities:

 Range GetRangeById(Repository repo, int id)
 {
    return repo.Query<Range>().SingleOrDefault(r=>r.Id == id);
 }

 void UpdateRange(Range rangeToUpdate, Range range)
 {
        // I'd even make this Range extension method
        rangeToUpdate.Status = range.Status;
        rangeToUpdate.FirstPsn = range.FirstPsn;
        rangeToUpdate.LastPsn = range.LastPsn;
        rangeToUpdate.PageCount = (int)(range.LastPsn - range.FirstPsn);
        rangeToUpdate.CourierId = range.CourierId;
        rangeToUpdate.WorkflowServer = range.WorkflowServer;
        rangeToUpdate.DoubleFeedInd = range.DoubleFeedInd;
 }

 void IAutoTisDal.UpdateRange(Range range)
  {
     using (var repo = _repositoryFactory.GetRepository(_baseline))
     {
        var rangeToUpdate = GetRangeById(range.Id);
        if (rangeToUpdate == null)
           throw new Exception(string.Format("Range ID {0} not found", range.Id));

        UpdateRange(rangeToUpdate, range);
        repo.Commit();
     }
  }

Two new methods are already easier to understand and test. And, honestly, I don't think that writing test for IAutoTisDal.UpdateRange makes much sense. It won't provide much value.

share|improve this answer
    
Thanks for the input; It does make it a bit easier on the eyes. But why wouldn't the test provide much value? I added the test after realizing I forgot to set the DoubleFeedInd property (Range class has a couple dozen properties). Perhaps I could have written the test to include only my missing property. –  hometoast Nov 13 '12 at 14:57
1  
I mean that after proposed refactoring this last method becomes quite simple. As I understand, tests provide value in (1) verify that newly written code works and (2) verify that it did not break after changes. In our case, last method is straightforward and relies on other (tested) methods. What can be verified there? That is throws an exception, or really updates db? –  Kefir Nov 13 '12 at 15:09
1  
Regarding forgotten property: your unit test won't detect the same bug when someone adds one more property to the Range entity. I think some metadata (reflection or data model or whatever - I'm not familiar with NHibernate, but I solved similar problem with Entity Framework) should be involved to verify that right properties are being copied (all public writeable except identity, probably). –  Kefir Nov 13 '12 at 15:14
    
I agree about the way to verify all properties are mapped/writable. In this case, the value of the other properties aren't important. –  hometoast Nov 13 '12 at 15:46
add comment

I dunno if I would use reflection for the range mapping to properties. I think this would be a great instance for Func<> lambda expressions. Or perhaps a dictionary key value implementation that can be iterated through..

share|improve this answer
    
Can you please give an example how lambdas or dictionary can help to retrieve all db-mapped properties? –  Kefir Nov 14 '12 at 6:15
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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