Code Review Quiz

I’ve been following Ayende’s series of reviews on the Microsoft N Layer App Sample V2.  His most recent post in the series explored a little bit of the data access layer in the app. Specifically, this interface (which has since been removed):

public interface IMainModuleUnitOfWork : IQueryableUnitOfWork
{
    #region ObjectSet Properties

    IObjectSet<BankAccount> BankAccounts { get; }
    IObjectSet<BankTransfer> BankTransfers { get; }
    IObjectSet<Country> Countries { get; }
    IObjectSet<Customer> Customers { get; }
    IObjectSet<Order> Orders { get; }
    IObjectSet<OrderDetail> OrderDetails { get; }
    IObjectSet<Product> Products { get; }
    IObjectSet<CustomerPicture> CustomerPictures { get; }
        
    #endregion
}

 

I couldn’t find the source for the IQueryableUnitOfWork base interface, but I assume it exposes a significant amount of methods and properties.

Ayende alluded to the fact that this interface has several significant problems with it, but didn’t go into detail.  So I thought that this would be a good opportunity for a quiz.  How many wrong things can you spot with this interface?

I counted at least 6 major problems, though I’m sure there are a few others.

Related Articles:

    Post Footer automatically generated by Add Post Footer Plugin for wordpress.

    About Chad Myers

    Chad Myers is the Director of Development for Dovetail Software, in Austin, TX, where he leads a premiere software team building complex enterprise software products. Chad is a .NET software developer specializing in enterprise software designs and architectures. He has over 12 years of software development experience and a proven track record of Agile, test-driven project leadership using both Microsoft and open source tools. He is a community leader who speaks at the Austin .NET User's Group, the ADNUG Code Camp, and participates in various development communities and open source projects.
    This entry was posted in .NET, code-review, quiz. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
    • Peter Johanson

      The three major ones I spot right off the bat:

      1) Violation of SRP.
      2) Violation of ISP.
      3) Returns the IObjectSet type everywhere, which leaks the details of EF into this “abstraction”. 

      Beyond that, this just “feels wrong” to me. Curious what specific issues others find that might help me understand the specifics of that “wrong feeling”.

      • http://chadmyers.lostechies.com Chad Myers

        Good call on ISP. I had missed that one! 

        1 Derives from a big base interface (I guess this is the ISP issue)2 Regions!3 Exposes EF IObjectSet instead of IQueryable or IList4 OCP violation (have to add to it every time you add to the app)5 SRP violation (too much going on)
        6 This interface, at least, is not a “unit of work” in the common definition of the pattern

    • http://twitter.com/ntcoding Nick

      I think the use of properties could also be an issue here. Or maybe not. I’m expecting logic that can throw exceptions and have side effects by operating on those properties.

      If I’m honest, I’d possibly do it the same way myself, though.

      I think Ayende would disagree, and accept the use of IObjectContext. I could be wrong, but he doesn’t like data access abstraction and I think in his post he condones this behaviour.

      I was expecting someone to chime in and say those entities aren’t aggregate roots, either. I would maybe expect order details to hang off orders, and customer pictures to hang off customers.

      Please don’t shout if I’m wrong.

      • Peter Johanson

        Yeah, I was wondering the same thing about the OrderDetails and CustomerPictures, whether they were true ARs or not.

    • Craig Cavalier

      Interestingly enough, I managed to find
      the IQueryableUnitOfWork interface – it was lerking in the namespace
      Microsoft.Samples.NLayerApp.Infrastructure.Data.Core. Digging deeper still, implementers
      of this interface also need to implement the underlying IUnitOfWork interface (which,
      to my surprise is apparently part of the “core domain” – Microsoft.Samples.NLayerApp.Domain.Core)
      and in turn,the ISQL interface (in the same namespace/project).

       

      As you predicted, there’s a multitude of methods
      here, including “commit”, “rollbackchanges”, “registerchanges”, “executequery”,
      “executecommand”, and “createset”.

       

      Let’s just say, I wouldn’t want to have to
      implement this interface if I wasn’t using EF. To be honest, I wouldn’t
      want to implement this interface regardless – we can do better. Of course, that
      would make an interesting follow up post.

    • Craig Cavalier

      Another issue with this interface is that it isn’t at all intention revealing, which of course, interfaces should be.If I were to hazard a guess at the reason for being of this interface, I would suggest that it only exists as a product of the EF tool, rather than there being a need for it  (This suspicion was somewhat confirmed when I realized it was generated code).

      Kinda seems silly now, picking apart the problems in generated code. Still a useful exercise in identifying code-smells mind you.

    • http://twitter.com/dingwallr Rich

      The domain being modelled here seems very odd to me. I cannot think what business scenario would bank accounts, tranfers, products and customer photos would all be first-class aggregate roots within a single bounded context. It’s either a domain I am unfamiliar with or (more likely, given the extremely vague name), a thinly-veiled 90′s style DAL.

    • Ryan means

      Hello Chad,
      I’m writing in the efforts of tring to find asp.net, c#, WPF deverloper for a client of mine in Austin. I’m here in Houston and normaly work on jobs in town.
      If you or anyone you know may be looking please let me know.
      Thanks,
      Ryan Means
      rmeans@vantageforce.com