After
being on holiday for a while and being pretty busy at work I want to talk about an upcoming issue at my current project. The issues are
- Presenter-Logic (Passiv View) is hard to test. It costs a lot of effort and it's difficult to be done right.
- There is code that is not worth being tested. We shouldn't test that code just to have the test coverage increased.
As a TDD advocate I'm trying to find out what's about those two issues above:
1.Passiv View is hard to test. Let see an example:
public void EditDienstAbwesenheit()
{
var abwesenheit = View.SelectedDienstAbwesenheit; // get selected item from view
if (abwesenheit != null) // if user as selected something
{
IModalDialog dialog;
if(abwesenheit is SpitalAufenthalt) // decide what type of item is selected
{ // create a modal dialog for this item type
var presenter = _viewBuilder.Create(_unitOfWork,typeof(ISpitalaufenthaltbearbeitenDialog));
dialog = presenter.View;
}
else
{ // creat a modal dialog for the other item type
var presenter = _viewBuilder.Create(_unitOfWork,typeof(IAbwesenheitbearbeitenDialog));
dialog = presenter.View;
}
if (Shell.ShowInModalWorkspace(View, dialog) != DialogResult.OK) // display the modal dialog
{
_unitOfWork.Refresh(abwesenheit); // reload data the might have been modified
}
UpdateView(); // show the modification on the view
}
}
We all know that the more dependencies and responsibilities a method has the more is it hard to test. This is like that because each dependency needs a
mock or stub and each responsibility needs a test-assertion to verify it. The example above has following responsibilities:
- Get the users selection
- Decide about the next dialog to be shown
- Create the dialog
- Open the dialog in the shell
- Reload to rollback modifications of the dialog
- Present the changes to the users
To accomplish that tasks we need:
- a View to get the users input
- a Model (called 'abwesenheit') that represent the users input
- a ViewBuilder to create a Dialog
- a Shell to display the Dialog
- a UnitOfWork to rollback the modifications
So what? Is it a test-problem or a design-problem? I strongly believe that code that can't be tested is poorly designed. The most obvious is that we violate the Single Responsiblity Principle (SRP). Could we split the responsibilities to find methods or even classes to separate the different concerns?
Lets try:
- Get the users selection -> Lets keep it where it is
- Decide about the next dialog to be shown -> Lets put it into a separate method
- Create the dialog -> Lets delegate it to another object as creation and displaying a dialog is often used in an application (e.g Application controller)
- Open the dialog in the shell -> move it to the Application controller
- Reload to undo modifications -> That's wrong here. The dialogs are responsible not to modify data in case they are cancelled!
- Present the changes to the users -> Lets keep it where it is
And after that minor refactoring:
public void EditDienstAbwesenheit()
{
var abwesenheit = View.SelectedDienstAbwesenheit;
if (abwesenheit != null)
{
CreateEditDialog(abwesenheit);
UpdateView();
}
}
public void CreateEditDialog(DienstAbwesenheit abwesenheit)
{
if(abwesenheit is SpitalAufenthalt)
{
_applicationController.ShowInModalWorkSpace(_unitOfWork,typeof(ISpitalAufenthaltBearbeitenDialog));
}
else
{
_applicationController.ShowInModalWorkSpace(_unitOfWork,typeof(IAbwesenheitBearbeitenDialog));
}
}
So is that better? I think yes. In terms of Unit-Testing we reduced responsibility and dependency per unit (method) and we got a cleaner more expressive design.
2. There is code that is not worth being testedLets see following example:
public class Person
{
public string FamilyName { get; set; }
public string Name { get; set; }
public string Age { get; set; }
}
public class Address
{
public string Code { get; set; }
public string Country { get; set; }
public string Street { get; set; }
}
/// <summary>
/// Model
/// </summary>
public class PersonRow
{
public PersonRow(Address address, Person person)
{
_address = address;
_person = person;
}
private readonly Person _person;
private readonly Address _address;
public string FamilyName
{
get { return _person.FamilyName; }
}
public string Name
{
get { return _person.Name; }
}
public string Age
{
get { return _person.Age; }
}
public string Code
{
get { return _address.Code; }
}
public string Country
{
get { return _address.Country; }
}
public string Street
{
get { return _address.Street; }
}
}
Is it worth to test that each getter returns the right value? When is it worth to test a piece of code? I could think of following answers:
- It's worth if something can go wrong
- It's worth if I don't need to put a lot of effort to test it
- It's worth if it's likely to be refactored in future
The problem with the example above is that little can go wrong (except the mapping) and that the effort is too high to test it. On the other hand as a TDD advocate I'm a little bit annoyed that this little stupid class should lower our test coverage. So what is the way out?
- If this class were not necessary there shouldn't go anything wrong
- If we could reduce the lines of code, less could go wrong and less needs to be tested
So let's try to introduce a mapper to externalize the mapping:
public class Person
{
public string FamilyName { get; set; }
public string Name { get; set; }
public string Age { get; set; }
}
public class Address
{
public string Code { get; set; }
public string Country { get; set; }
public string Street { get; set; }
}
/// <summary>
/// Model
/// </summary>
public class PersonRow
{
public string PersonFamilyName { get; set; }
public string PersonName { get; set; }
public string PersonAge { get; set; }
public string AddressCode { get; set; }
public string AddressCountry { get; set; }
public string AssressStreet { get; set; }
}
public void DoTheMapping()
{
var person = new Person();
var address = new Address();
var row = new PersonRow();
PropertyMapper.To(row).From(person).From(address).Execute();
}
The idea is to eliminate all the boring mappings to a specialized class PropertyMapper. The PropertyMapper would map the values from person and address to the row combining Classname and Propertyname. Furthermore the method Execute() would throw an exception in case any property of the PersonRow was not initialized with a value. My observations are:
- The PropertyMapper is difficult to implement, but I think after 10 mappings i have already my return on investment
- Execute() is self-validating, therefore this code can also be run under test
- I split the responsibilities into two classes (PersonRow and PropertyMapper) and I got testability
- The example should be extended to support two way mappings
- Is that too much?
Posts
