patterncsharpMinor
Does my code need refactoring before adding unit tests?
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
```
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
{
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
Make it default to an empty list, and you would not need to check for nulls. Less
-
Factor out the repeating chunks about related organizations and
-
Duplication is bad in itself. The following two snippets look like they were copy/pasted; then an error was fixed, for just one copy.
-
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 collectionif (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.