# Is this architecture overly complex or following best practices?

I work as a C# developer at a company that doesn't use best practices at all. We're on .NET 3.5 but most code is written in a .NET 1.1 style (e.g. almost all the logic is in the code behind of the ASPX page, everything uses untyped DataSets instead of returning objects, makes gratuitous use of Session and QueryString to pass data, very little architectural patterns). I have spent a couple of years learning best practices by reading blogs and watching screencasts from the top echelon of .Net developers.

I was recently tasked with creating a new feature on our in-house ERP system that writes data from XML to a PDF for display. It was suggested that we might want various types of PDFs in the future, so the framework should be flexible. I took the opportunity to apply proper design patterns and a DDD-like approach to the code, with the idea being to demonstrate a "better way" of doing things to my peers. My code was subsequently rejected in a "code review" by the lead developer/development manager as being "too complicated" compared to simply writing all the logic in a single class or in the code-behind (what my other co-workers would have done had they been given the project instead). I managed to get the code passed anyways as they didn't want me to "waste time" going back and changing it (they consider refactoring a waste of time that adds no business value), but it got me thinking if the code really is too complex; I appear to be following all of the best practices, I am following (I think) all of the SOLID principles, I am using proper design patterns and OOP software engineering techniques.

Here is a simplified version of the classes (may or may not be 100% compilable as it's a stripped down version; you should get the idea though):

// Repositories
public interface IRepository { }
public interface IXmlRepository : IRepository {
XmlDocument GetXml();
}

// Repositories.Impl
public abstract class BillingXmlRepository : IXmlRepository {
protected long quoteID;

public BillingXmlRepository(long quoteID) {
this.quoteID = quoteID;
}

public abstract XmlDocument GetXml();
}

public sealed class XmlInvoiceRepository : BillingXmlRepository {
public XmlInvoiceRepository(long quoteID) : base(quoteID) { }
public override XmlDocument GetXml() {
// XML retrival here...
}
}

// DataAggregation
pubilc interface IAggregatableData<T> {
T GetData();
}

public abstract class DataAggregator<T> : IAggregatableData<T> {
public abstract T GetData();
protected abstract ICollection<IRepository> GetAllRequiredRepositories();
}

public abstract class XmlDataAggregator : DataAggregator<XmlDocument> {
public override XmlDocument GetData() {
XmlDocument root = new XmlDocument();
foreach (IRepository repository in this.GetAllRequiredRepositories()) {
XmlDocument xml = (repository as IXmlRepository).GetXml();
XmlDocumentFragment fragment = root.CreateDocumentFragment();
fragment.InnerXml = xml.InnerXml;
root.DocumentElement.AppendChild(fragment);
}
return root;
}
}

public sealed class BillingDataAggregator : XmlDataAggregator {
private long quoteID;

public BillingDataAggregator(long quoteID) {
this.quoteID = quoteID;
}

protected override void ICollection<IRepository> GetAllRequiredRepositories() {
return new List<IRepository> {
new XmlBillingRepository(this.quoteID);
}
}
}

// Mappers to map raw XML to classes
public interface IMappable<T, K> {
T Map(K rawData);
}

public interface IXmlMappable<T> : IMappable<T, XmlDocument> {
}

public sealed class BillingXmlMapper : IXmlMappable<BillingInfo> {
public BillingInfo Map(XmlDocument rawData) {
// LINQ code to traverse the XML and map it into a BillingInfo DTO
}
}

// Entities
public sealed class BillingInfo {
// simple properties
}

// Consumer example
XmlDataAggregator aggregator = new BillingDataAggregator(1234);
var xml = aggregator.GetData();
var mapper = new BillingXmlMapper();
BillingInfo info = mapper.Map(xml);


Am I on the right track as far as following the best way to write modular code? I can't bring myself to ignore good practices and write all the code in code-behind files or the like.

• Did you show your peers any unit tests that you were able to write as a result of segregating the interfaces? That might have gone some way towards convincing them – MattDavey Aug 27 '13 at 8:39
• It's a moot point now as I was fired from this job in July '12 for applying design patterns to the code and trying to help my peers improve, but we had no unit tests of any kind at the company and testing was seen as a waste of time. – Wayne Molina Aug 27 '13 at 14:56
• Sorry to hear that, I didn't realise this question was so old. I hope you're in a more inspiring environment now. – MattDavey Aug 27 '13 at 18:32

I think you are complicating it a bit too much.

The first question that pops into my head is, "Why so much inheritance?" Why inherit IXmlRepository from IRepository? What's the point?

I'd also take issue with calling that a "Repository" - I think that is one of the most overused terms in architecture at the moment. Read Evans' DDD or Fowler's site and tell me if you really think that class qualifies under a definition of "Repository". And then convince me that no one will get confused between that and some other type of "Repository" that exists in your project. (just going from my experiences here)

Also regarding inheritance, why the abstract DataAggregator? I understand you've cut down from the actual code, but from what I can see here it is again a totally pointless construct.

Another one: you have an inheritance chain that is XmlInvoiceRepository -> BillingRepository -> IXmlRepository -> IRepository. Huh? Why on earth is XmlInvoiceRepository inherited from BillingRepository? Hopefully not just because Invoices are part of Billing in someone's conceptual model.

Is there some reason XmlInvoiceRepository -> IXmlRepository wouldn't be enough?

Also, you're throwing a lot of sealed's in there. Why? Are there clear reasons for that choice, or is it more just for good measure? If the latter, get rid of it. Be as simple as possible and no simpler.

Also, do you really need two interfaces to represent a function that can map from an XmlDocument to a generic type? How many places is that really getting used. Same question with your DataAggregator, how many different places is that really getting used.

Your usage example is right on. That's what you should aim for. Short, concise, clear, to the point. The operations are occurring on the same level of abstraction.

I would just say that you seem to be unnecessarily creating a bit too many artifacts in the process that don't appear to provide any actual value.

• You're right, and after I posted this I consolidated it a little bit; for instance I only have the IXmlRepository<T>, not an IRepository and then an IXmlRepository that doesn't do anything except say I'm using an XmlDocument. I made some other changes I can't remember (rather, I can't remember what I obfuscated for the code snippet here :p). The sealed is something that I only recently started after reading something from the Java world about when to use final on classes. – Wayne Molina May 5 '11 at 19:50

I agree with qes on all points but in summary you've just taken the Interface Segregation principle a little too far. But the most important point I want to get across is consider a career move. You've demonstrated a willingness to learn so don't hang around in a team that's not willing to assist you in this.

The interfaces might be a little overkill, but they're not necessarily a bad idea. This is how the code is meant to be written. If it's too complicated for the lead and potentially the rest of the team, then perhaps the company needs a new team. I have to architect things with our junior developers in mind with respect to our applications, but on our team we always establish the needs of the project against the resources available to train up our juniors to be able to handle certain things.

That being said, we do have a few people here who just "bang stuff out", and then they end up having to maintain it because they've failed to follow industry standards so the rest of us have to re-engineer it their way before we can troubleshoot.

My advice to you is to continue to lead by example doing things according to industry standards, but don't become a maverick bucking every trend set by those above you. You could end up coding yourself out of a job. Continue keeping up with best practice and current trends, continue to rally for these things, but don't just begin implementing things because you think they're the right way. Your description of this situation sounds like you handled it just right. They asked you to do something, and you did it according to best practices and industry standards. If they had other requirements, they should have included those instructions up front.

I think that DDD might be a little too much for such a small task. You don'treally need a repository to hold a in memory collection of data since you are not dealing with a datastore per say, only transfering data from one output to another.

You could use the OOP design patterns for this for sure, look at the seperation of concerns and use IoC if you like and strategy pattern to encapsulate some algorithms, just don't go overboard and apply too many design patterns. :)

• +1 for "DDD might be a little too much for such a small task.". However there's really no sign of DDD in the example anyway... (having a class called "Repository" doesn't make it DDD) – MattDavey Aug 27 '13 at 8:45