SPTDD: On Good Vs. Testable Code

First, my position on SharePoint Test Driven Development: I don’t currently use it. I got a free Isolator license (thanks!) that I have yet to install (sorry!). Just like everyone else, I’m trying to figure out where TDD fits in the context of SharePoint. Any assertions in this post about TDD are based on my current understanding, which is incomplete at best.

This post is in response to a post by Eric Shupps: SPTDD: SharePoint and Test Driven Development, Part One. He has a lot to say, so let’s start with this assertion:

…in order to get real value from TDD in SharePoint you must already know how to write good code. All the unit tests in the world won’t change this fact. And the only way to learn how to write good code is to do it over and over again, gathering knowledge along the way from those who have gone before you. This leads the the fundamental problem with TDD as a methodology – it doesn’t teach developers how to write good code; rather, it teaches them how to write testable code.

I agree with the differentiation between good and testable code, but I think Eric underestimates the value of testable code in relation to its goodness. For reference, let’s bring in how his example code “should be written”:

private SPListItemCollection GetListItems(string Url, string ListName)
{
    SPListItemCollection coll = null;
    try
    {
        if (Url != String.Empty && ListName != String.Empty)
        {
            SPSecurity.RunWithElevatedPrivileges(delegate()
            {
                using (SPSite site = new SPSite(Url))
                {
                    using (SPWeb web = site.OpenWeb())
                    {
                        try
                        {
                            SPList list = web.Lists[ListName];
                            if (list.Items.Count > 0)
                            {
                                coll = list.Items;
                            }
                        }
                        catch (System.Exception ex)
                        {
                            LogError(ex.Message);
                        }
                    }
                }
            });
        }
    }
    catch (System.Exception ex)
    {
        LogError(ex.Message);
    }
    return coll;
}

I know “good code” is subjective, but this method has some real problems:

  • SP* objects should not be passed out of a RWEP block.
  • SP* objects should not be used after their parent SPWeb has been disposed.
  • Each call to list.Items will execute a new query. Instead, store it to a variable or use list.ItemCount.

These issues have nothing to do with testability. They’re simply not good SharePoint code. But TDD wouldn’t fix them, so for the purpose of argument let’s use this code instead (essentially Eric’s original plus error handling):

private SPListItemCollection GetListItems(string Url, string ListName)
{
    SPListItemCollection coll = null;
    try
    {
        if (Url != String.Empty && ListName != String.Empty)
        {
            SPSite site = new SPSite(Url);
            SPWeb web = site.OpenWeb();
            try
            {
                SPList list = web.Lists[ListName];
                SPListItemCollection items = list.Items;
                if (items.Count > 0)
                {
                    coll = items;
                }
            }
            catch (System.Exception ex)
            {
                LogError(ex.Message);
            }
        }
    }
    catch (System.Exception ex)
    {
        LogError(ex.Message);
    }
    return coll;
}

So supposing we used TDD, a method like this would have been created based on tests written to verify the following:

  • If Url or ListName are empty, return null.
  • If Url or ListName are null, use them anyway and log the resulting exception.
  • Create SPSite and SPWeb that the caller needs to remember to dispose.
  • If the list has no items, return null, otherwise return all items.
  • If anything goes wrong, log the exception and return null.

I would guess this isn’t exactly what Eric intended the method to do (at least regarding null arguments), but the hypothetical tests drove the TDD implementation; a different set of tests would have led to a more correct version of the method. Herein lies the problem with critiquing TDD based on code developed without tests—TDD would likely yield a different implementation!

Abstraction and Dependency Injection = Artifacts?

I also take issue with this passage:

Look closely – the code itself satisfies all the test requirements without requiring any level of abstraction, dependency injection, nUnit, or other such artifices.

[Irony: artifice - n. Cleverness or skill; ingenuity.]

Regarding satisfaction of the requirements, the obvious question is: How do you know? Without a test, you don’t, and to think you do just because you “already know how to write good code” is more naive than thinking unit-tested SharePoint code is bulletproof. Case in point: null arguments. But more importantly, when did abstraction and dependency injection become TDD things? These are basic programming principles, completely independent from TDD or SharePoint! Just because we’re developing against a specific (and complicated) API doesn’t mean the general best practices don’t apply. We should be .NET developers first, SharePoint developers second. Even without using TDD, I submit that your particular code could benefit quite a bit from dependency injection:

private SPListItemCollection GetListItems(SPWeb web, string listName, SPQuery query)
{
    if (web == null || string.IsNullOrEmpty(listName))
        return null;
    SPListItemCollection coll = null;
    try
    {
        SPList list = web.Lists[listName];
        SPListItemCollection items = query == null ? list.Items : list.GetItems(query);
        if (items.Count > 0)
        {
            coll = items;
        }
    }
    catch (Exception ex)
    {
        LogError(ex.Message);
    }
    return coll;
}

Instead of creating an SPWeb that needs to be disposed by the caller, we just accept one that the caller should already be handling. Instead of always using list.Items (not recommended), we accept an optional SPQuery. The method has a clear purpose, doesn’t have the new SPSite to handle, works just as well with an elevated SPWeb, etc. It’s simply a better design, with improved testability being more of a side benefit than the main goal. I can’t say for sure, but I would guess a TDDed version (given the right requirements) would be similar.

On Mocking

Again, I haven’t found time to try out Isolator so I can’t really speak to the actual practice of testing mocked objects; however, it seems to me that it would be useful to verify that, given certain assumptions about a SharePoint environment, tests pass. If I can copy/paste a few lines of code to fake an SPWeb, with or without a named list, with or without list items, this seems like it would be much easier than actually creating and deleting lists and items to verify that my code behaves accordingly. Then as long as I’m in an environment that matches an assumption I tested, I’m assured the code will work as expected. Isolator seems to enable this sort of test with relative ease, at which point the general arguments for (and against) testing and TDD can be applied to SharePoint without much translation.

All that said, thanks to Eric for posting his thoughts to spark a more public discussion. Also, check out Andrew Woodward‘s first rebuttal and other posts on SharePoint and TDD (particularly Unit Testing SharePoint Solutions – Getting into the SharePoint Object Model).

About these ads

12 Responses to “SPTDD: On Good Vs. Testable Code”

  1. Peter Says:

    Yeah I was in that discussion. I’m learning TDD outside of work, and when it’s possible, it’s beautiful. And I don’t just mean “beautiful” in the aesthetic sense, I mean the experience is beautiful. I’m a newbie and it’s already helping.

    Meanwhile, at work I do build in some level of abstraction because I don’t like the standard procedural way of doing things. Were this my code, I would have something like the following–and note I’ve invented a reason for this list to exist, because he didn’t give any context as to why his code ran at all:

    public class LoggingList
    {
      private readonly SPList _list;
      public LoggingList(SPList list)
      {
        _list = list;
      }
      public IEnumerable<LogItem> GetAllItems()
      {
        foreach (SPListItem item in _list.Items)
        {
          yield return new LogItem(item);
        }
      }
    }

    With this setup, the object holds onto its own SPList reference, and that’s it. Disposing the parent SPWeb/SPSite objects is handled elsewhere, in what may possibly be an ugly manner :).

    I’d be interested to see how Bil Simser does this kind of stuff; he was in the comments thread explaining his approach, which frankly sounds better than what I have written above.

  2. Peter Says:

    OK so as expected, the code sample didn’t format great. No problem, just note that I meant to do IEnumerable[LogItem], and have some indentation.

    Also I don’t necessarily recommend my approach, it’s just “what I’m doing right now.”

  3. Keith Dahlby Says:

    Hey Peter ~

    As usual, the problem with fabricated examples is that it’s not clear what else LoggingList does. However, my initial thought was “what if the instance gets passed out of _list‘s SPWeb scope accidentally”? Particularly because the iterator will defer execution. So if I have something like this…
    LoggingList ll;
    using(SPWeb web = site.OpenWeb())
    {
      ll = new LoggingList(web.Lists["MyLog"]);
    }
    foreach(LogItem li in ll.GetAllItems()) { // oops

    Of course, if you have other uses for _list and its SharePoint dependency is well-documented, that’s fine. But in a lot of transformative cases like this, I tend to think more functionally – list in, projection out – for which extensions methods are handy:

    static class LoggingExt {
      static SPQuery LogQuery { get {
       return new SPQuery { ViewFields = "..." };
      }}
      static IList<LogItem> GetLogItems(this SPList list) {
        return (from SPListItem i in list.GetItems(LogQuery)
                select new LogItem(i)
               ).ToList();
      }
    }

    I wish WordPress had better options for code in comments. An editor like StackOverflow would be fantastic.

  4. Peter Says:

    Interesting. I’m still working with VS2005 (we’re …just about to roll out VS2008…) so I haven’t been able to mess with extension methods yet. It would be interesting to see how this would look on a grander scale (and no I’m not telling you to expose your proprietary stuff :) )

    I did attend a Matt Podwysocki presentation and he mentioned he’s built a generic Using(Func) function, so if you want to go totally functional, check it out!

  5. Keith Dahlby Says:

    Since VS2008 can build against .NET 2.0 and 3.0, you really should upgrade ASAP to get access to C# 3. Also for targetting pre-3.5, check out LINQBridge.

    I’m not quite in the “everything functional” camp, but I do find myself using LINQ and lambdas more and more. If I want to abstract the using from the handling logic, I do something like this:

    static void ProcessWeb(string url, Action<SPWeb> action) {
      using(SPSite site = new SPSite(url))
      using(SPWeb web = site.OpenWeb())
        action(web);
    }
    static void DoSomething() {
      ProcessWeb("http://wss", w =>
      {
        // Do something with w
      });
    }

    There are more examples in my post on elegant elevation, but if there’s anything in particular you’d like to see functionalized I’ll certainly give it a try.

  6. Keith Dahlby Says:

    Or a more testable version:
    static void DoSomething() {
      ProcessWeb("http://wss", DoSomethingWithWeb);
    }
    static void DoSomethingWithWeb(SPWeb w) {
      // Do something with w
    }

  7. Thinking Functional: Using « Solutionizing .NET Says:

    [...] Functional: Using February 20, 2009 — Keith Dahlby In the comments of my last post, Peter Seale pointed me to Matthew Podwysocki’s implementation of GenerateUsing as functional [...]

  8. Jeremy Thake Says:

    Really pleased that you have cleared this up. I think Eric’s comments are a bit misleading, but the opinion is definately in parts a valid discussion to be had.

    You may also want to take a look on the SharePointDevWiki.com page on unit testing too.

    http://www.sharepointdevwiki.com/display/public/SharePoint+Development+with+Unit+Testing

  9. Jonas Says:

    Great post!

    1) Be explicit, don’t trust your caller throw ASAP when you don’t get what u expect
    2) Again don’t allow wrong arguments
    3) Hate null
    3.1) Hate null
    3.2) Hate null
    4) A function returning a collection SHOULD NEVER return null but an empty collection

    private SPListItemCollection GetListItems(SPWeb web, string listName, SPQuery query)
    {
    if (web == null )
    {
    throw new ArgumentNullException(“web”);
    }
    if( string.IsNullOrEmpty(listName) )
    {
    throw new ArgumentException(“listName”);
    }

    SPList list = web.Lists[listName];
    SPListItemCollection items = query == null ? list.Items : list.GetItems(query);

    SPListItemCollection coll = items;
    return coll;
    }

    • Keith Dahlby Says:

      I was trying to model the example on Eric’s original, but in general I agree with all your points. If I recall correctly, indexing into Lists with a missing list name throws a rather obscure error, which I might wrap in a more useful exception.

      There are dozens of variations on this kind of method; through TDD you would only write precisely the method you need.


Comments are closed.

Follow

Get every new post delivered to your Inbox.

%d bloggers like this: