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 {
protec

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.

StackExchange Code Review Q#2259, answer score: 5