What a cruel fate, to be Ctrl-V'd

Questions like this pop up now and again over at StackOverflow and usually I find myself agreeing with the majority; copy-paste coding leads to smells and all sorts of nastiness. In our current code base here, I have had the joy task of refactoring some C# code that looks something like:

switch (caseName) {
  case "First":
    // Many lines of boilerplate
    break;
  case "Second":
    // Many lines of the same boilerplate with a single value changed
    break;
  // and so on
}

Inexcusable? Perhaps. However, sometimes the reality of business hits and you've got a day to implement a new feature for a demo tomorrow and right about then you start clinging to the Ctrl-C chord pretty hard. After all, shipping the product is a feature. So sometimes you suck it up and paste that switch statement together and hope for the best.

There is a line I like to draw, and that is when you start duplicating code that you don't understand. Recently I stumbled across the following blog post detailing a method to hide the checkbox from arbitrary items in a TreeView. I'll duplicate the important part of the code here (C#):

TVITEM tvItem = new TVITEM();

tvItem.hItem = node.Handle; 
tvItem.mask = TVIF_STATE;
tvItem.stateMask = TVIS_STATEIMAGEMASK;
tvItem.state = 0;

IntPtr lparam = Marshal.AllocHGlobal(Marshal.SizeOf(tvItem));
Marshal.StructureToPtr(tvItem, lparam, false);
SendMessage(this.treeView1.Handle, TVM_SETITEM, IntPtr.Zero, lparam);

What's missing here? Why that's correct, we're missing a call to Marshal.FreeHGlobal since MSDN says we need to free that memory. Now this is just sample code so you might be tempted to say that it is the reader's burden to interpret and understand the code (and I would agree with you). Looking at the comments to this post however, I see many individuals that seem to have taken the code literally and no doubt used it without thinking through it. As if further proof was needed I recently found nearly that exact same code polluting our own code base and fixed it. Now many of our developers are new to C#, and indeed, to Windows programming in general so I can't entirely fault them. I just find it hard to consider oneself a software developer (or moreso, an engineer) when actual design and engineering principles are ignored as often as they are in this field.

In case you were wondering, the correct code for hiding a checkbox on a TreeView doesn't need to allocate any unmanaged memory directly at all:

public const int TVIF_STATE = 0x8;
public const int TVIS_STATEIMAGEMASK = 0xF000;
public const int TV_FIRST = 0x1100;
public const int TVM_SETITEM = TV_FIRST + 63; 

[StructLayout(LayoutKind.Sequential)]
public struct TVITEM  {
  public int mask;
  public IntPtr hItem;
  public int state;
  public int stateMask;
  [MarshalAs(UnmanagedType.LPTStr)]
  public string lpszText;
  public int cchTextMax;
  public int iImage;
  public int iSelectedImage;
  public int cChildren;
  public IntPtr lParam;
}

[DllImport("user32.dll")]
static extern IntPtr SendMessage(IntPtr hWnd, uint Msg, IntPtr wParam, ref TVITEM lParam);

private void Hide_Checkbox(TreeNode node) {
  TVITEM tvi = new TVITEM();
  tvi.hItem = node.Handle;
  tvi.mask = TVIF_STATE;
  tvi.stateMask = TVIS_STATEIMAGEMASK;
  tvi.state = 0;
  SendMessage(this.Mission_treeView.Handle, TVM_SETITEM, IntPtr.Zero, ref tvi);
}

It's almost funny to note that this code is both simpler and more efficient (although I haven't proved it) than the other (broken) code.