Does my code need refactoring before adding unit tests?

Question

I have this code that works fine, but I would like to create some unit test for this class. Most of the code is in private or void type methods. Please help me refactor my class to make it more testable.

namespace Insperity.HCRBlackBox.HCRServices.Biz
{
    public class OrganizationChartBiz : IOrganizationChartBiz
    {
        private readonly OrganizationChartRepository _repository;

        public OrganizationChartBiz()
        {
            _repository = new OrganizationChartRepository();
        }

        public AcknowledgeOrganizationChartType NotifyOrganizationChart(NotifyOrganizationChartType notifyOrganizationChartType)
        {
            foreach (var organizationChart in notifyOrganizationChartType.DataArea.OrganizationChart)
            {
                foreach (var unit in organizationChart.OrganizationUnit)
                {
                    var existing = GetExistingOrganization(unit.OrganizationUnitID.Value);
                    if (existing.Equals(null))
                    {
                        TransformToOrganizationAndSave(unit);
                    }
                    else
                    {
                        TransformToOrganizationAndUpdate(unit, existing);
                    }
                }
            }
            return new AcknowledgeOrganizationChartType();

        }

        public AcknowledgeOrganizationChartType SyncOrganizationChart(SyncOrganizationChartType syncOrganizationChartType)
        {

            foreach (var organizationChart in syncOrganizationChartType.DataArea.OrganizationChart)
            {
                foreach (var unit in organizationChart.OrganizationUnit)
                {
                    var existing = GetExistingOrganization(unit.OrganizationUnitID.Value);
                    if (existing!=null)
                    {
                        TransformToOrganizationAndUpdate(unit, existing);
                    }
                    else
                    {
                        TransformToOrganizationAndSave(unit);
                    }
                }
            }

            return new AcknowledgeOrganizationChartType();
        }

        private void TransformToOrganizationAndUpdate(OrgChartOrganizationUnitType org, Organization existing)
        {
            List<RelatedOrganization> relatedOrganizations = new List<RelatedOrganization>();
            var now = DateTime.UtcNow;

            var organization = existing;
            if (org.ParentOrganizationUnit.OrganizationUnitID.Value!=null)
                organization.ParentOrganizationId = org.ParentOrganizationUnit.OrganizationUnitID.Value;
            organization.OrganizationName = org.OrganizationUnitName.Value;
            organization.LastModified = now;

            if (org.RelatedOrganizationUnit != null)
            {
                foreach (var relatedOrganizationUnitType in org.RelatedOrganizationUnit)
                {
                    var controlGroup = new RelatedOrganization
                        {
                            ControlGroupId = relatedOrganizationUnitType.OrganizationUnitID.Value,
                            StartDate =
                                Convert.ToDateTime(relatedOrganizationUnitType.OrganizationUnitID.validFrom),
                            Organization = organization,
                            EndDate =
                                relatedOrganizationUnitType.OrganizationUnitID.validTo != null
                                    ? Convert.ToDateTime(relatedOrganizationUnitType.OrganizationUnitID.validTo)
                                    : new DateTime?(),
                            LastModified = now
                        };
                    relatedOrganizations.Add(controlGroup);
                }
            }

            if (org.UserArea != null)
            {
                var userArea = org.UserArea as OrganizationChartUserArea;
                if (userArea != null)
                {
                    organization.EducationIndicator = userArea.EducationIndicator.Equals(false) ? "F" : "T";
                }
            }

            _repository.UpdateOrganization(organization,relatedOrganizations);
        }

        private void TransformToOrganizationAndSave(OrgChartOrganizationUnitType org)
        {
            var now = DateTime.UtcNow;

            var organization = new Organization
            {
                OrganizationId = org.OrganizationUnitID.Value,
                ParentOrganizationId = org.ParentOrganizationUnit.OrganizationUnitID.Value,
                OrganizationName = org.OrganizationUnitName.Value,
                LastModified = now
            };

            if (org.RelatedOrganizationUnit != null)
            {
                foreach (var relatedOrganizationUnitType in org.RelatedOrganizationUnit)
                {
                    var controlGroup = new RelatedOrganization
                    {
                        ControlGroupId = relatedOrganizationUnitType.OrganizationUnitID.Value,
                        StartDate =
                            Convert.ToDateTime(relatedOrganizationUnitType.OrganizationUnitID.validFrom),
                        OrganizationId = relatedOrganizationUnitType.OrganizationUnitID.Value,
                        EndDate =
                            relatedOrganizationUnitType.OrganizationUnitID.validTo != null
                                ? Convert.ToDateTime(relatedOrganizationUnitType.OrganizationUnitID.validTo)
                                : new DateTime?(),
                        LastModified = now
                    };
                    organization.RelatedOrganizations.Add(controlGroup);
                }
            }
            if (!org.UserArea.Equals(null))
            {
                var userArea = org.UserArea as OrganizationChartUserArea;
                if (userArea != null)
                    organization.EducationIndicator = userArea.EducationIndicator.Equals(false) ? "F" : "T";
            }

            _repository.SaveOrganization(organization);
        }

        private Organization GetExistingOrganization(string organizationUnitId)
        {
            return _repository.GetExistingOrganization(organizationUnitId);
        }

        public void Dispose()
        {
            _repository.Dispose();
        }
    }
}

Answer 1

In addition to @palacsint's advice, namely you should make the implicit dependencies of system under test explicit, I'll add a few more.

• In my experience easiest way to make a business method that uses system time testable is to just pass the current time as a parameter. "Time" is not part of your domain after all. This refactoring is a form of Replace Static Variable with Parameter This will also save you the trouble of setting up a mock time provider service for test scenarios.

• The following means org.RelatedOrganizationUnit is some kind of collection

  if (org.RelatedOrganizationUnit != null)
  {
     foreach (... in org.RelatedOrganizationUnit)
  

  Make it default to an empty list, and you would not need to check for nulls. Less ifs, less test cases needed.

• Factor out the repeating chunks about related organizations and EducationIndicator out smaller units are easier to test.

• Duplication is bad in itself. The following two snippets look like they were copy/pasted; then an error was fixed, for just one copy.

  // this should throw NullReferenceException if org.UserArea is null
  if (!org.UserArea.Equals(null)) 
  
  if (org.UserArea != null)
  

Answer 2

I'm not too familiar with C#, so just a few generic notes:

1. If you can, use a dependency injection (DI) framework and inject the OrganizationChartRepository instead of creating it inside the constructor. It would make the classes less coupled and therefore easier to test. If you can't use DI, you can create another constructor:

   @VisibleForTesting
   public OrganizationChartBiz(OrganizationChartRepository repository) {
       _repository = repository;
   }
   

   In the tests you can create the OrganizationChartBiz instance with a mocked repository and verify how the OrganizationChartBiz instance uses the mock. (@VisibleForTesting is from Guava. I guess C# has something similar or you can create your own markup for marking code which is exists only for the sake of testing.)

2. Instead of DateTime use a TimeSource or Ticker interface to wrap the DateTime. See: Use a fake system clock. You can pass this ticker to the constructor and pass a mocked ticker in the tests to make the behaviour repeatable.