Friday 29 March 2013

[DataMember] Hides Dead Code From ReSharper

18 months ago I wrote a post describing how unit tests obscure dead code. The solution I found was to unload all the test projects and see what ReSharper then threw up. I’ve since discovered another case where ReSharper will not warn you about unused members - the [DataMember] attribute.

If you’ve done any serialization in C# I’m sure you’ve come across the pair of attributes DataContract and DataMember. You annotate your classes [1] with them like so to allow them to be serialized (e.g. with WCF):-

[DataContract]
public class MyType
{
  [DataMember]
  public string MyValue { get; private set; }
}

Whilst doing the refactoring I mentioned in my last post one of the changes I was going to make was to remove the [DataMember] attribute from a property that did not need to be serialized because it was a cached value. A few seconds after removing the attribute, the name when dark grey and ReSharper pointed out it was not used. Somewhat bemused I undid my change and lo and behold ReSharper stopped pointing out it was redundant.

I can only imagine (or I could read the manual) that this behaviour is because ReSharper has to assume the property is accessed via reflection and so in some codebases it’s likely to report a false positive. To a degree we already have this problem with the classes we use with the Plossum command line parser because they are only poked by reflection, although I set a default value in the constructor in this case to placate ReSharper/FxCop.

For the moment, until I follow my own advice and RTFM, I’m just temporarily commenting out the [DataMember] attributes on all properties and waiting a few seconds to give ReSharper time to think. Then I put the attributes back again where R# thinks they are in use. Of course they may only be in use because of the unit tests, which is quite likely because I always write a unit test to verify that a class correctly supports serialization [2]. However I’m hopeful that those clever JetBrains developers will one day allow me to have my cake and eat it too.

 

[1] I’ve never been entirely comfortable with this approach to serialization as it feels wrong to be adding the responsibility for serialization to the class itself. In C++ your serialization code was often orthogonal to the class itself as you might read and write multiple formats. These two attributes don’t seem overly invasive but the XML serialization attributes certainly appear to be.

[2] All it does is create an instance of the class, serialize and deserialize with DataContractSerializer and then compare the properties against the values used to construct the original instance. Any derived properties are also verified to ensure that an internal OnDeserialized() method has been added where necessary.

Thursday 28 March 2013

Property vs Method - Ownership Semantics

I was recently doing a spot of refactoring and came across some code that used an object that was backed by a native resource in a buggy way. I can see how the situation came about but have not so far come across any description of a convention that suggests how it should be done. Perhaps it’s blindingly obvious to everyone else, but in case it’s not or my argument is flawed, this is my take on the matter...

The Bug

Imagine you have a C# managed type that is a wrapper around a native type which, because it can be very large and is native needs managing carefully in the C# world using the Dispose pattern. If you want something more concrete imagine you have a managed library that wraps a native library to allow you to load web pages and manipulate the Document Object Model (DOM).

public class DOM : IDisposable
{
  public static DOM LoadFile(string filename)
  {
    // Invoke native code to load file.
  }

  public void Dispose()
  {
    // Invoke native code to cleanup.
  }
}

So, given that this is a resource that generally needs careful management [1] the default stance will be to apply a Using block to ensure it’s correctly cleaned up at the end of use, whether that be through normal or exceptional circumstances:-

using (var dom = DOM.LoadFile(filename))
{
  // Manipulate the DOM to your hearts content…
}

Now, what about when you don’t create this resource directly but instead you acquire it from another object, perhaps via a property or a method. Absent any documentation or looking at the property/method implementation would you expect to take ownership or not? For the sake of the example code I’m going to suggest that the type from which we acquire this DOM is another C# managed class called WebPage [2].

var page = new WebPage(. . .);

// Access DOM from a property - taking ownership?
var dom = page.DOM;

// Access DOM from a method - taking ownership?
var dom = page.ToDOM();

The code I came across was written like this:-

using (var dom = page.DOM)
{
  // Manipulate the DOM and directly dispose it
}

But the implementation of the WebPage’s DOM property looked like this:-

public class WebPage
{
  private DOM m_dom = null;

  public DOM
  {
    get
    { 
      if (m_dom == null) 
        m_dom = CreateDom(); 

      return m_dom;
    }
  }
}

Naturally, being legacy code, the container class didn’t implement the Dispose pattern itself and so you could never have managed the resource properly even if you wanted to. But I’m in the middle of a refactoring and wandering what to do next. Plus, if possible, I want to lay down (or better yet promote an existing one) a convention about when ownership is likely to be transferred. And, when it isn’t what the containing class needs to do instead.

ToXxx() Method Passes Ownership

In the particular refactoring I was looking at the class was really acting like a Builder [2] and so I changed the property to a method and renamed it ToXxx(), where Xxx was the type. Essentially it was a type conversion, like ToString(), and that also meant ownership should be passed to the caller. This also meant I didn’t have to fix the code structure of the buggy caller; I only had to do the renaming.

Going back to my example above this leads to the following implementation and style of invocation:-

public class WebPage
{
  public DOM ToDom()
  {
    return CreateDom();
  }
}

// In the caller
using (var dom = page.DOM)
{
  // Manipulate the DOM and directly dispose it
}

Naturally this is the simplest implementation for both the container and caller.

Xxx Property Retains Ownership

In the past I’ve also found myself taking the property approach. A property is usually a class attribute held by containment and the obvious question might be why something this complex was being exposed directly in the first place. In the places I’ve used it’s because the containing class is essentially a Facade over an underlying complex object. In an idea world it would remain purely an implementation detail but legacy code (until refactored) often demands access to the underlying object in the mean time.

In these cases I have decided to retain ownership for performance reasons and expose the legacy object through a property. This is what the WebPage class in the example above was alluding too. But what it missed was the necessity of implementing the Dispose pattern itself so that it could delegate cleanup to the underlying object at the right time. So it should have looked like this:-

public class WebPage : IDisposable
{
  private DOM m_dom = null;

  public DOM
  {
    get
    { 
      if (m_dom == null) 
        m_dom = CreateDom(); 

      return m_dom;
    }
  }

  public void Dispose()
  {
    if (m_dom != null)
      m_dom.Dispose();
  }
}

// In the caller
using (var page = new WebPage(. . .))
{
  var dom = page.DOM;

  // Manipulate the DOM and indirectly dispose it
}

This is a little more effort for the callee and a fair bit more for the containing class. It probably seems a lot more effort to C# developers who are only used to managing simple resources like files and database connections in a limited scope. Annoyingly in C++ you have types like std::shared_ptr<> that just make this a nonissue, in fact after const it’s probably the second biggest thing I miss.

I would have hoped that a static analysis tool like ReSharper would have pointed out the need to implement IDisposable originally, but it doesn’t. I don’t remember FxCop picking it up either but I’ve not looked at FxCop recently and I suspect both would generate way too much noise for most normal users. Perhaps there is a setting I need to turn on instead? What I’m after (should a more knowledgeable person be reading this) is a warning when one type aggregates another (IDisposable) type and doesn’t implement IDisposable itself. In library and server-side code I assume that any type implementing IDisposable always requires its lifetime to be tightly controlled as you have no idea how tight or lapse the constraints are for the hosting process.

 

[1] Yes, in many cases where the objects are quite small we can let the garbage collector weave “it’s magic” and be blissfully unaware of the memory pressures created by the native goings on. But in many cases these babies are multi-megabyte monsters that have a serious impact on the (32-bit) process’s footprint.

[2] Try and ignore for the moment the possibly obvious (to you) thought that it looks like the Builder pattern and therefore ownership semantics are probably obvious. This is legacy code here, these rules don’t apply until after I’ve refactored it.

Thursday 21 March 2013

You Want Windows 98 Support in 2013?

A few weeks back I got an email from a chap in Australia who wanted to know if I could fix one of my DDE tools (DDE Command) to work under Windows 98 SE. After a quick check of the calendar to make sure I hadn’t entered a time warp I couldn’t help but be a little curious about where this mysterious Windows box was running and why it couldn’t be upgraded…

It turned out that this machine was fitted with a propriety ISA card (remember those?) for which support was discontinued in 1996! This chap seemed to be going through heroic efforts to keep it running by using Excel to grab data from the card via DDE. He then stumbled upon my command line tool which worked fine on Windows 7 (as it does right back to Windows 2000), but was failing on Windows 98 like this:-

C:\> ddecmd servers
ERROR: Failed to query DDE servers: DMLERR_DLL_NOT_INITIALIZED

I had not seen that kind of error before and it’s pretty fundamental too - the underlying DDEML library was apparently not initialised. All ddecmd commands seemed to report the same problem. Luckily I had more than an inkling of what it might be because the tool is pretty simple and my DDE classes have barely changed in over decade. Also I didn’t really fancy trying to cobble together a VM and source a Windows 98 licence just to help this chap out. One of the reasons I provide the source code to all my tools is exactly to allow someone to find their own (paid) support if I can’t accommodate them myself.

One of the key differences between the Windows 95/98/ME lineage and the NT/2K/XP/Vista/etc one is that the former is ANSI internally whereas the latter is Unicode. Naturally for backwards compatibility reasons the NT line can also run ANSI binaries too, with a slight overhead as it translates back and forth as required. One of the decisions I made back in February 2008 was to switch to Unicode builds by default; however the Windows build targets were still left as they were when I first ported my libraries to Win32 back in the mid ‘90’s!

#define WINVER         0x0400   //! Windows 95+
#define _WIN32_WINNT   0x0400   //! Windows NT 4.0+
#define _WIN32_WINDOWS 0x0400   //! Windows 95+
#define _WIN32_IE      0x0400   //! IE 4.0+

Luckily I have no need for anything more fancy, especially with my command line tools so it was a simpler matter of flicking the switch in Visual C++ (7.1, aka VS2003, is still my favoured version) and out popped an ANSI build. In the meantime because of the time lag between the UK and Australia I suggested to the chap that he download my GUI based DDE tool (DDEQuery) and try that. I’d remembered that this tool hasn’t been touched since 2005 and so the binary would have been an ANSI build anyway. Unsurprisingly, it worked. So I shipped off an ANSI build of DDE Command and it was “case closed”.

Every time I think it’s time to leave the past behind and get-with-the-program a question like this pops up and it feels great to still be able to support such an old OS. Although I still use VC++ 7.1 (on XP) for my personal C++ code, I still compile it with modern versions of VC++ and GCC to gain access to all that extra static analysis so that someone can just pick it up if needs be. My day job now consists of writing C# and although it’s currently .Net 3.5 I have my sights firmly set on .Net 4.5 and C# 5 as I’m no luddite.

Even though the corporate desktop standard is still Windows XP in many organisations and we now have to suffer all those annoying “you’re using an outdated browser version” banners, it feels good to know that there are others out there that have it so much worse.