HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Does my code need refactoring before adding unit tests?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
refactoringneedtestsaddingdoescodebeforeunit

Problem

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
{

Solution

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)

Code Snippets

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

if (org.UserArea != null)

Context

StackExchange Code Review Q#28979, answer score: 6

Revisions (0)

No revisions yet.