Code comments

Jul 29, 2007 at 7:14 AM
Not sure if you're planning on changing anything, as the code is evolving as you write the book (and my comments might not make sense) but I thought I would ask some questions here about the code as it stands right now.

1. In BuildFactoryTest you call an EntityFactoryBuilder to create an IEntityFactory of <Project>. Just wondering why you don't have it create an instance of IEntityFactory<IProject>. I would probably have two interaction tests here. One that EFB created a factory of type IEntityFactory and that IEntityFactory<IProject> created an instance of IProject (which could all be done in mocks).

2. There are a couple of Mocks in the UnitTests.Mocks folder. Just wondering if you planning to use Rhino or something to get rid of these? As with above, if everything is coded to interfaces, Rhino might make more sense than having people create their own mocks (which are really stubs IMHO).

3. The tests all are for Visual Studio unit testing. Just as a suggestion or idea, maybe make it unit test agnostic (the attributes are almost identical across MS Test, NUnit and MBUnit) and maybe have special test projects for each type (similar to how CAB provides both MS Test and NUnit test projects).

4. In ProjectRepository test you have FindAll, FindBy(Key) and FindAllMarket segments. Will you be introducing a specification to cut down on the number of Find methods?

5. You might want to introduce some base interfaces that are more DDD sounding like IEntity, IAggregate, IAggregateRoot. Even if they're simple (like only holding a Key object) I think it might add more context to the code over EntityBase everywhere.

6. The domain is already getting big with Projects, Contacts, Users, MarketSegment, MarketSelector, etc. but there's very few tests for any of these and the domain model is looking a little anemic? Or maybe you're not driving out the domain with TDD?

Just curious and wondering about where things are going. Looks good so far and looking forward to another DDD book out there. Never can have enough.
Coordinator
Jul 31, 2007 at 9:16 PM
Bil,

Those are all excellent comments/suggestions. Thank you for taking the time to go over my code.

I will try to answer all of your questions if I can:

1. I decided not to use interfaces for my domain objects (Project, Contact, etc.) because I just did not see any value in that. My domain objects have no persistence code in them, they are POCO's, and therefore should be very easy to write tests against without having to dummy them up.

2. The only things I am coding to interfaces at this point in the application are my repositories and factories. I also have a single interface for my Unit of Work implementation as well as my Views.

3. That's a great idea about adding the NUnit tests like patterns & practices did with EntLib. I like how they did that too. I really need to put more work into my tests.

4. Although I am an advocate of the Specification pattern, I made a design decision to not use it as an argument to my repository methods. Instead, I have chosen to make the methods on my interfaces very explicit. I think this adds more clarity to the code I am writing. I have not noticed too many FInd methods, and so far, it seems to be working out pretty well as far as code maintenance goes.

5. That's a good idea...I have gone back and forth on that and I really wanted to make my classes as POCO as possible (although the EntityBase class is a layered super type, and it does help eliminate a lot of common code). I chose EntityBase because I knew I needed a way to identify my entities, but using an interface isn't a bad idea. I could just make the EntityBase class implement it, and then any method argument that took an EntityBase could take an IEntity instead.

6. I am lacking on tests...I tried to do TDD, but I am not really that good at it. I like having my intellisense (as opposed to writing tests for things that do not yet exist). It's just not natural for me to think that way. But I do need a lot more unit test coverage, I admit I am sucking badly in that category. What else about my domain looks anemic? My focus was on making it as POCO as possible, but maybe my objects are not rich enough, although I am trying to make them rich. Do you have any suggestions for helping the domain be more supple?

Thanks again for you comments, they have been very well-received.

Tim
Oct 26, 2007 at 4:16 PM
Hi Tim

Having a chance to peruse your code (I saw your slides from your post and added a comment or two) I would have to tend to agree with bsimser that the domain model is a little anaemic.

By anaemic we mean : http://martinfowler.com/bliki/AnemicDomainModel.html:

The basic symptom of an Anemic Domain Model is that at first blush it looks like the real thing. There are objects, many named after the nouns in the domain space, and these objects are connected with the rich relationships and structure that true domain models have. The catch comes when you look at the behavior, and you realize that there is hardly any behavior on these objects, making them little more than bags of getters and setters.

I looked and did find a piece of business logic in RoutingItem::DateReturned, so I'm sure you don't intend the domain to be void of business logic, you just don't have any yet so it' looks a bit anaemic.

I would add some tests around the calculation of DaysLapsed. Adding a few tests may also highlight your intentions behind allowing return dates before ship dates in your domain..

e.g.
...
RoutingItem item = new RoutingItem(key, discipline, routingOrder, recipient, sentToday, returnedTomorrow);
item.DateReturned = yesterday;
Assert.AreEqual(1, item.DaysLapsed); //hmm
...

This is of course a state based test. When there is further business logic, erm, contrived example off the top of my head:

When a companies headquarters address changes a change of address notification is sent to finance through a service class.

Your unit test for this should be testing the Company class in isolation so you'd have an interaction based test and the easiest way to do that is using an interface to the service and mock it with a mocking framework like Rhino mocks. As the business logic in your classes grows/evolves these interaction based tests start to warrent interfaces between your domain classes when you mock them out.

It's probably not applicable to you here as it's an example but I have been leaning towards deliberately creating the classes for one to many relationships for some time now (you can still use generics) for example:

public class Addresses : List<Address>
{
}

This seems odd as an empty class but sooner or later your going to want an operation or business logic that acts on all the items and putting that code in the class referencing it is a violation of the single responsibility principal (not to mention you may have many List<Addess> scattered throughout the domain model). This is more to do with maintenance over time though so not doing this to keep the class numbers down to keep the example small is perfectly valid.

Apologies for rambling on, it's good stuff, I normally have much much more to say about designs I see :-)

One last thing: Your DataContract.Helper.Converters look very much like assemblers see Fowler DTOs , is this the intention? might be more intention revealing to call them as such if that's the case

I hope that was coherent I was typing as I looked

Regards

Ed Hill



Coordinator
Feb 14, 2008 at 5:39 AM
Edited Feb 14, 2008 at 5:48 AM
Ed,

Sorry it took me so long to get back to you, I was under some pretty intense deadlines with the book. First of all, thank you for taking the time to review my code, I really appreciate it! That is a great idea about creating the classes for one to many relationships...I guess I did not do it yet due to the YAGNI principle, but I could see that as a refactoring happening later.

As far as the model being a little anemic, I really tried to focus on correcting that, and my last couple of code drops show some good behavior now in the domain model. I will also be adding more tests as well.

For the Data Contract Converters, that's a great idea! I think I will make them more like Fowler's assemblers...as soon as I can dedicate some time to this again :)

Thank you,

Tim


EdHill wrote:
Hi Tim

Having a chance to peruse your code (I saw your slides from your post and added a comment or two) I would have to tend to agree with bsimser that the domain model is a little anaemic.

By anaemic we mean : http://martinfowler.com/bliki/AnemicDomainModel.html:

The basic symptom of an Anemic Domain Model is that at first blush it looks like the real thing. There are objects, many named after the nouns in the domain space, and these objects are connected with the rich relationships and structure that true domain models have. The catch comes when you look at the behavior, and you realize that there is hardly any behavior on these objects, making them little more than bags of getters and setters.

I looked and did find a piece of business logic in RoutingItem::DateReturned, so I'm sure you don't intend the domain to be void of business logic, you just don't have any yet so it' looks a bit anaemic.

I would add some tests around the calculation of DaysLapsed. Adding a few tests may also highlight your intentions behind allowing return dates before ship dates in your domain..

e.g.
...
RoutingItem item = new RoutingItem(key, discipline, routingOrder, recipient, sentToday, returnedTomorrow);
item.DateReturned = yesterday;
Assert.AreEqual(1, item.DaysLapsed); //hmm
...

This is of course a state based test. When there is further business logic, erm, contrived example off the top of my head:

When a companies headquarters address changes a change of address notification is sent to finance through a service class.

Your unit test for this should be testing the Company class in isolation so you'd have an interaction based test and the easiest way to do that is using an interface to the service and mock it with a mocking framework like Rhino mocks. As the business logic in your classes grows/evolves these interaction based tests start to warrent interfaces between your domain classes when you mock them out.

It's probably not applicable to you here as it's an example but I have been leaning towards deliberately creating the classes for one to many relationships for some time now (you can still use generics) for example:

public class Addresses : List<Address>
{
}

This seems odd as an empty class but sooner or later your going to want an operation or business logic that acts on all the items and putting that code in the class referencing it is a violation of the single responsibility principal (not to mention you may have many List<Addess> scattered throughout the domain model). This is more to do with maintenance over time though so not doing this to keep the class numbers down to keep the example small is perfectly valid.

Apologies for rambling on, it's good stuff, I normally have much much more to say about designs I see :-)

One last thing: Your DataContract.Helper.Converters look very much like assemblers see Fowler DTOs , is this the intention? might be more intention revealing to call them as such if that's the case

I hope that was coherent I was typing as I looked

Regards

Ed Hill





Mar 17, 2008 at 9:01 PM

Tim

Snap.

I've been too busy to have a look at what you've changed (actually I'm cooking dinner while I repy here) :-)

I've got a mentoring session coming up and as they are starting to cover unit testing I was reminded of your code base and thought it might be a good idea to get them to flesh out the unit test for your domain model. Since it won't be production code, I'd be able to forward it on to you if you've not managed to get much coverage going with your unit tests.

As an aside I remembered you used data driven unit testing and I wanted to show the group I'm mentoring how although it has it's place for certiain types of testing, it's drawback is you can't see the intention from the test.

So I fired up the domain model project and did a few project searches : "if (", "for " "while"

and I didn't get much back.... (the odd null test and a little bit of validation)

If you don't have much logic in the domain model it will unfortunately look a bit too much like actve record

I'm probably missing it, so I'll look at bit further

Regards

Ed
.


{quote}
tmccart1 wrote:
Ed,

Sorry it took me so long to get back to you, I was under some pretty intense deadlines with the book. First of all, thank you for taking the time to review my code, I really appreciate it! That is a great idea about creating the classes for one to many relationships...I guess I did not do it yet due to the YAGNI principle, but I could see that as a refactoring happening later.

As far as the model being a little anemic, I really tried to focus on correcting that, and my last couple of code drops show some good behavior now in the domain model. I will also be adding more tests as well.

For the Data Contract Converters, that's a great idea! I think I will make them more like Fowler's assemblers...as soon as I can dedicate some time to this again :)

Thank you,

Tim

{quote}
EdHill wrote:
Hi Tim

Having a chance to peruse your code ... <snip>
{quote}
Apr 3, 2008 at 2:09 AM
Hi Tim, Ed,

<<This seems odd as an empty class but sooner or later your going to want an operation or business logic that acts on all the items and putting that code in the class referencing it is a violation of the single responsibility principal (not to mention you may have many List<Addess> scattered throughout the domain model). This is more to do with maintenance over time though so not doing this to keep the class numbers down to keep the example small is perfectly valid.>>

Whilst I would agree on this, and I do love spouting SRP and all those others documented in Uncle Bob's book, because I do love playing the sanctimnious git at times! :-) Your point makes sense, but what I would have to question Ed on is what kind of operations do you envisage doing to all the items?, I guess I'm asking how would you protect these methods from corruption from outside of the aggregate root? Please forgive me if I've missed something obvious here.

I have an app I'm working on at the moment that is littered with IList<T>, so would be interested to hear your thoughts.

Cheers
Colin Basterfield
Coordinator
Apr 20, 2008 at 8:30 PM
Here are my my thoughts on deliberately building the classes to model the one to many relationships (i.e. Addresses : List<Address>):

I agree that it is a good idea, but I really did not need it yet, so I followed the YAGNI principle. When the time comes, I will refactor to it, and let the unit tests guide me to the right implementation :). I am sure it will eventually come up, but it just turns out hat it has not yet.

Thanks,

Tim
Apr 22, 2008 at 9:53 PM

Hi Colin

I got no email notification about your post until Tim responsed. I guess the second notification pushed the first one through (although that makes little sense)

So apologies for the delay in responding

So for addresses, lets say that as a business rule a customer is only allowed 5 addresses or because a contract note needs printing for each address and will be delivered by gilded carriage, there needs an explicit check that no adresss added is a repeat address. (two gilded carriages to the same address causes no end of trouble I'm sure). The path of least resistence is to put this logic in addesses.

Looping through, you got anything from applying discounts to lineitems in an order to the allocation of commission to underwriters of a initial public offering (although I see a strategy pattern in that's future but only when another allocation method comes along)

Anytime a client of the code requires a for each with a bit of code in it, you can easily evaluate whether or not it would be useful to have a specific method that promotes the operation with a name or leave the calling code iterating through adding x and subtracting y, it always depends. It's almost always easier to unit test those sort of things in isolation without the calling codes other stuff getting in the way too.

It's easier to change it's inne working too. A specific example recently was changing the storage from a list to a dictionary so that lookup for calculations was much more performant, an IList was still exposed, (It made no sense to force the other consumers to deal with KeyValuePairs) and everything still continued to work. Someone will no doubt say that some key combination of resharper would handle this situtation :-)

Small development team, (or just you coding) then YAGNI rules but in larger teams where Junior developers are cutting their teeth on minor changes to existing systems or a developer has 8 changes to get through before the next release, the path of least resistance for those developers has a huge impact on their decisions on which class to add their code.

Maybe I've become too cynical :-)

Ed




ColinBasterfield wrote:
Hi Tim, Ed,

<<This seems odd as an empty class but sooner or later your going to want an operation or business logic that acts on all the items and putting that code in the class referencing it is a violation of the single responsibility principal (not to mention you may have many List<Addess> scattered throughout the domain model). This is more to do with maintenance over time though so not doing this to keep the class numbers down to keep the example small is perfectly valid.>>

Whilst I would agree on this, and I do love spouting SRP and all those others documented in Uncle Bob's book, because I do love playing the sanctimnious git at times! :-) Your point makes sense, but what I would have to question Ed on is what kind of operations do you envisage doing to all the items?, I guess I'm asking how would you protect these methods from corruption from outside of the aggregate root? Please forgive me if I've missed something obvious here.

I have an app I'm working on at the moment that is littered with IList<T>, so would be interested to hear your thoughts.

Cheers
Colin Basterfield

Coordinator
Apr 23, 2008 at 11:28 PM
One thing I just thought of as an alternative solution is to use Specifications. I actually use then quite a bit in this project to enforce relationship constraints and other business rules. That may be why I did not put in an Addresses class, a lot of times I would just use a Specification to enforce the integrity.

Tim


EdHill wrote:

Hi Colin

I got no email notification about your post until Tim responsed. I guess the second notification pushed the first one through (although that makes little sense)

So apologies for the delay in responding

So for addresses, lets say that as a business rule a customer is only allowed 5 addresses or because a contract note needs printing for each address and will be delivered by gilded carriage, there needs an explicit check that no adresss added is a repeat address. (two gilded carriages to the same address causes no end of trouble I'm sure). The path of least resistence is to put this logic in addesses.

Looping through, you got anything from applying discounts to lineitems in an order to the allocation of commission to underwriters of a initial public offering (although I see a strategy pattern in that's future but only when another allocation method comes along)

Anytime a client of the code requires a for each with a bit of code in it, you can easily evaluate whether or not it would be useful to have a specific method that promotes the operation with a name or leave the calling code iterating through adding x and subtracting y, it always depends. It's almost always easier to unit test those sort of things in isolation without the calling codes other stuff getting in the way too.

It's easier to change it's inne working too. A specific example recently was changing the storage from a list to a dictionary so that lookup for calculations was much more performant, an IList was still exposed, (It made no sense to force the other consumers to deal with KeyValuePairs) and everything still continued to work. Someone will no doubt say that some key combination of resharper would handle this situtation :-)

Small development team, (or just you coding) then YAGNI rules but in larger teams where Junior developers are cutting their teeth on minor changes to existing systems or a developer has 8 changes to get through before the next release, the path of least resistance for those developers has a huge impact on their decisions on which class to add their code.

Maybe I've become too cynical :-)

Ed




ColinBasterfield wrote:
Hi Tim, Ed,

<<This seems odd as an empty class but sooner or later your going to want an operation or business logic that acts on all the items and putting that code in the class referencing it is a violation of the single responsibility principal (not to mention you may have many List<Addess> scattered throughout the domain model). This is more to do with maintenance over time though so not doing this to keep the class numbers down to keep the example small is perfectly valid.>>

Whilst I would agree on this, and I do love spouting SRP and all those others documented in Uncle Bob's book, because I do love playing the sanctimnious git at times! :-) Your point makes sense, but what I would have to question Ed on is what kind of operations do you envisage doing to all the items?, I guess I'm asking how would you protect these methods from corruption from outside of the aggregate root? Please forgive me if I've missed something obvious here.

I have an app I'm working on at the moment that is littered with IList<T>, so would be interested to hear your thoughts.

Cheers
Colin Basterfield


Apr 25, 2008 at 2:22 PM
That's a very good point Tim

Specification is quite flexible, selecting subsets, validation etc, whenever you need a test to see if an object matches some criteria.

Like strategy, it allows you have great flexibility but I'd be concerned with implementing every rule using specifications overusing it can make the code harder to read and understand, it's a fine line though.

I'm still leaning towards specific inclusion. For example the ChangeOrder class in your model composes a number specification, this isn't supplied to it by the caller of that class, it's part of it's logic.

Whereas specifications applied to an IList are defined by the calling code which is external. It doesn't seem alien to have a select by specification method on a repository class but no such method on a collection class seems ok to a lot of people....

I forgot to answer colin's questions about protection them outside the aggregate root. My answer is that you'd do it much the same way as normal. Since access is allowed only through the aggregate root object you would pass back an isolated (probably cloned) object disconnected from the repository. Typically the result for me is going to end up in a DTO but if your concerned about the logic that resides in the collection class passing outside the aggregate then by all means use an IList.

Good point through, I'm advocating using specific collection classes in the domain model, not necessarily for consumers of that model :-)

Regards

Ed
May 2, 2008 at 4:31 PM
I mentioned the data driven unit tests before. As an approach for more integration like testing it's ok but it's hard to see from just reading the tests what is going on.

For example:

DeploymentItem("SmartCA.sdf"), TestMethod()
public void PreviousTimeChangedTotalTest()
{
object projectKey = new Guid("5704f6b9-6ffa-444c-9583-35cc340fce2a");
int number = 5;
ChangeOrder target = new ChangeOrder(projectKey, number);
int actual = target.PreviousTimeChangedTotal;
Assert.AreEqual(4, actual);
}

This probably gets pretty good code coverage per lines of test code :-) but how that value of 4 arrives is a mystery.

Here's it rewritten with a bit of tweaking, I'm using Rhino Mocks as the mock framework:

private MockRepository mocks;
private IChangeOrderService mockChangeOrderService;

/// <summary>
/// Ensures we have clean objects on each test
/// </summary>
TestInitialize()
public void StartUp()
{
mocks = new MockRepository();
mockChangeOrderService = mocks.CreateMock<IChangeOrderService>();
}

/// <summary>
/// The test gets it's value from the ChangeOrderService
/// </summary>
TestMethod()
public void PreviousTimeChangedTotalRetrivedFromChangeOrderService()
{
using (mocks.Record())
{
Expect.Call(mockChangeOrderService.GetPreviousTimeChangedTotalFrom(null)).IgnoreArguments().Return(4);
}
using (mocks.Playback())
{
ChangeOrder target = new ChangeOrder(mockChangeOrderService); //constructor injection but you can use an IoC container instead
Assert.AreEqual(4, target.PreviousTimeChangedTotal);
}
}

/// <summary>
/// The mock allows more expressive tests like this one
/// </summary>
TestMethod()
public void PreviousTimeTotallsOnlyRetreivedOnce()
{
using (mocks.Record())
{
Expect.Call(mockChangeOrderService.GetPreviousTimeChangedTotalFrom(null)).IgnoreArguments().Return(4);
}
using (mocks.Playback())
{
ChangeOrder target = new ChangeOrder(mockChangeOrderService);
Assert.AreEqual(4, target.PreviousTimeChangedTotal);
Assert.AreEqual(4, target.PreviousTimeChangedTotal);
Assert.AreEqual(4, target.PreviousTimeChangedTotal);
}
}

I only needed to make a couple of changes to ChangeOrder and the ChangeOrderService classes to enable this.

In situations where your testing for error conditions, it's much easier to test like this and it also allows you to show you fully intend never to go get the value from the service again for a particular instance of ChangeOrder.

This method of unit testing gives you the opportunity to really exercise your logic in those specification pattern classes as well....

Regards

Ed