4
\$\begingroup\$

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();
        }
    }
}
\$\endgroup\$
6
  • 2
    \$\begingroup\$ 1. I'd change the wording in your question. "Please help me refactor" gives the impression that you want other people to do your work for you. 2. I recommend reading the book Working Effectively with Legacy Code by Michael Feathers for tips on how to introduce tests to a codebase. \$\endgroup\$
    – Lstor
    Jul 25, 2013 at 15:30
  • \$\begingroup\$ @Lstor I'm not asking anyone to write my unit test just to help make the code more testable. I hope I didn't give you that impression. \$\endgroup\$ Jul 25, 2013 at 15:50
  • 1
    \$\begingroup\$ I know, that's why I suggest the rewording :) Otherwise I would have just downvoted. \$\endgroup\$
    – Lstor
    Jul 25, 2013 at 15:52
  • \$\begingroup\$ @Lstor Thanks, I took your advice. Hopefully the new title is more accurate. \$\endgroup\$ Jul 25, 2013 at 15:56
  • \$\begingroup\$ Names are confusing, collections not having plural etc... Does a NotifyOrganizationChartType message target a domain entity. You seem to be missing an aggregate. What does the collection of organizationCharts that is the notifyOrganizationChartType.DataArea.OrganizationChart signify in the domain, to the user for example? \$\endgroup\$ Jul 26, 2013 at 11:10

2 Answers 2

6
\$\begingroup\$

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)
    
\$\endgroup\$
1
  • \$\begingroup\$ Thanks you make great points. The only one I can't do is default RelatedOrganizationUnit to empty. I do not own that class and am not the source of the data, this is a wcf service. Thanks for the advice \$\endgroup\$ Jul 26, 2013 at 13:25
5
\$\begingroup\$

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.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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