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):// Repositoriespublic interface IRepository { }public interface IXmlRepository : IRepository {     XmlDocument GetXml();}// Repositories.Implpublic 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...    }}// DataAggregationpubilc 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 classespublic 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    }}// Entitiespublic sealed class BillingInfo {     // simple properties}// Consumer exampleXmlDataAggregator 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.
Is this architecture overly complex or following best practices?
c#;design patterns;xml
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.