TestFixture attribute – just because R# doesn’t care, doesn’t mean you shouldn’t

TestFixture attribute – just because R# doesn’t care, doesn’t mean you shouldn’t

Recently I noticed ReSharper’s built-in test runner doesn’t require you to decorate your fixtures as TestFixture — it’s smart enough to locate Test methods regardless. My team noticed this too, and as a result we’ve started to omit TestFixture entirely.

Turns out this is a big mistake. NUnit itself requires them — in our case, we discovered our build server (which calls NUnit directly) was ignoring a large number of tests (including some that were failing), and was reporting much lower code coverage results than expected.

So, somehow we need to enforce that the TestFixture attribute is applied everywhere it should be, so us lazy ReSharper developers don’t miss anything. Not to worry… we can write a test for that!

[TestFixture]public class MissingTestFixtureAttributeTests{    [Test]    public void All_test_fixtures_must_have_test_fixture_attribute()    {        IEnumerable<Type> badFixtures =            from t in Assembly.GetExecutingAssembly().GetTypes()            where HasTests(t)            where IsMissingTestFixtureAttribute(t)            select t;        ErrorIfAny(badFixtures);    }    static bool HasTests(Type type)    {        var testMethods =            from m in type.GetMethods()            from a in m.GetCustomAttributes(typeof (TestAttribute), true)            select m;        return testMethods.Any();    }    static bool IsMissingTestFixtureAttribute(Type type)    {        var attributes =            from a in type.GetCustomAttributes(typeof (TestFixtureAttribute), true)            select a;        return !attributes.Any();    }    private static void ErrorIfAny(IEnumerable<Type> fixtures)    {        if (!fixtures.Any())            return;        var sb = new StringBuilder("The following types are missing [TestFixture] attributes:");        sb.AppendLine();        foreach(Type type in fixtures)        {            sb.AppendLine();            sb.Append(type);        }        throw new Exception(sb.ToString());    }}

libcheck – quick and dirty assembly compatibility debugging

libcheck – quick and dirty assembly compatibility debugging

Today I wrote a little command-line tool for helping track down .NET assembly compatibility issues:

System.IO.FileLoadException: Could not load file or assembly {0} or one of its dependencies. The located assembly's manifest definition does not match the assembly reference.

My team encounters these problems very regularly — we have a build/dependency tree that easily rivals Castle Project in complexity, and it gets out of sync easily while waiting for downstream projects to build. Normally, your only option in this situation is to fire up reflector and poke through all your references manually — a very tedious process.

So, to use this tool, you point it at a directory (e.g. your lib folder) and provide a list of patterns for assemblies you want to check:

libcheck.exe C:devyourapplib *NCover*

Then it’ll tell you any that are broken.

Mandatory overloads

Mandatory overloads

Yesterday I read a pretty sensible tweet by Jeremy D. Miller:

(args), you almost always need a Method(Type, args) too.’ width=’550′ height=’292′>

In spirit, I would like to propose another:

When you have a method like Method(IEnumerable<T> items), you should always provide a Method(params T[] items) too.

Access invocation arguments when returning a value with Rhino Mocks

Access invocation arguments when returning a value with Rhino Mocks

My team use Rhino Mocks at work, and as a Moq fan, one of my most missed features is the ability to access invocation arguments when returning a value. For example:

mock.Setup(x => x.Execute(It.IsAny<string>()))    .Returns((string s) => s.ToLower());

Rhino lacks this feature out of the box. It is possible, but pretty ugly:

mock.Stub(x => x.Execute(Arg<string>.Is.Anything))    .WhenCalled(invocation =>        invocation.ReturnValue =             ((string) invocation.Arguments[0]).ToLower());

Today I wrote some quick extensions for Rhino to make it behave a bit more like Moq.

mock.Stub(x => x.Execute(Arg<string>.Is.Anything))    .Return<string, string>(s => s.ToLower());

Grab them here: RhinoExtensions.cs

Duct-tape programmers ship… once

Duct-tape programmers ship… once

I read an interesting article today full of praise for duct-tape programmers, a breed of developers who are smart enough to get away without unit tests, and ship code early by using duct-tape style programming techniques.

Duct-tape programmers are considered great because they help companies take first-mover advantage, while others waste time developing abstract frameworks and trying to over-engineer everything. Customers and project managers love duct-tape programmers because they get the job done fast.

I see duct-tape programmers in a different way:

Of course you need to ship products fast to beat your competition. But duct-tape isn’t a sustainable way to build software, unless you have the luxury of never having to touch the code again (one-off console games come to mind here). Otherwise, maintenance costs from brittle code will soon come back to bite and affect your ability to ship version 2.0 in time.

Our challenge is to find a balance in the middle where we aim high, but have an architecture flexible enough to allow duct-tape-style compromises in a clean and isolated fashion when required.

Domain entities vs presentation model objects

Domain entities vs presentation model objects

This is the first half of a two-part article. Read the second half here: Domain entities vs presentation model objects, part 2: mapping.

When putting domain driven design (DDD) principles to practice in a real application, it’s important to recognize the difference between your domain model and presentation model.

Here’s a simple real-life example from a little MVC application that displays a to-do list of tasks. The task domain entity is very simple, with an identifer, a name and an optional due date:

// task domain entitypublic class Task{    public int Id;    public string Name;    public DateTime? DueDate;    // ...etc}

Pretty much everything we want to know about this Task can be derived from its properties via Specifications. For example, an OverDueTaskSpecification will tell us whether or not the task is overdue by checking if the due date has already passed, and an UnscheduledTaskSpecification will tell us if the task is scheduled by checking if the due date is null.

However, when rendering the task to the user, the application’s view must remain dumb — a passive view — and cannot work this sort of stuff out for itself. It is not enough to simply pass a collection of domain entities to the view; all relevant details must be explicitly provided so the view has all the information it requires without having to do any work itself.

Together, all these UI-specific details form a presentation model object, which is effectively identical to a DTO in that it has only carries data and no methods or behaviour of its own (some people call them PTOs). Here’s what my application’s presentation model object for a task looks like:

// task presentation model objectpublic class TaskView{    public int Id;    public string Name;    public string DueDate;    public bool IsUnscheduled;    public bool IsOverDue;    public long SortIndex;}

The ID and name fields are exactly the same, but DueDate is now a string which will either hold a friendly-formatted date or ‘Anytime’ if the task is not scheduled. Unscheduled and overdue are now explicit flags so the view can immediately identify tasks that need special display like highlighting.

Most of the front-end user interaction in this application is implemented via JavaScript, so we need an index for sorting and appending tasks in the correct order. It needs to be simple to parse and fast to compare, so I chose a long integer which is resolved from the due date.

Note this is a very simple example with only one domain entity class which we just added some new properties to. Most real scenarios will require some degree of flattening aggregate object graphs into a single class, and hiding of fields which are not relevant.

In the big scheme of things, presentation model objects are defined in the application layer (see the Onion Architecture), where they are validated and mapped between domain entities. All public interfaces to the application layer are expressed in terms of presentation model objects, so domain entities do not leak into the UI layer.

In an ASP.NET MVC application for example, presentation model objects would be used as the Models for all views and action results.

Update: a friend asked me total why I don’t format DueDate in the view, as proper MVC separation of concerns would dictate. The reason is that as well as rendering them directly in the view when a page is requested, this application also sends presentation model objects to the client via AJAX. I decided that having one single place (instead of two) where tasks’ DueDates are formatted would make maintenance easier in the long run.

Back to basics: exception handling in .NET

Back to basics: exception handling in .NET

Following on from my article on good source control check-in habits, I’ve got a few tips I’d like to share on exceptions in .NET. These are basically all responses to things I’ve seen done in production code before.

Write a method that throws — don’t return success/failure codes

Here’s a C function I wrote a couple of years ago for an application a couple of years back that listens for messages via UDP:

int bind_udp_server(struct udp_server * server){        server->socket_descriptor = socket(AF_INET, SOCK_DGRAM, 0);        if (-1 == server->socket_descriptor)                return -1; /* bail out */                if (!set_non_blocking(server->socket_descriptor))                return -1;  /* bail out */                ...                if (-1 == bind(server->socket_descriptor,                        (struct sockaddr *) &server_addr,                        sizeof(server_addr)))                return -1; /* bail out */                return 0; /* socket bound successfully */}

C has no formal mechanism for functions to return to their caller when exceptional behaviour is encountered. Instead they typically return an integer status code (e.g. 1 or 0) to indicate success or failure.

Clients using these functions must check for errors after every call. Because of this:

  • Your real application code can be much harder to follow when it’s hidden amongst all the error handling code (which probably only executes in rare circumstances anyway).
  • If you accidentally omit a return code check and something fails, your program will continue happily executing as if nothing happened. Problems might not become apparent until several operations later, making it much harder to track down the original issue.
  • Because you have to check for errors the whole way (and at different levels), there will be a lot of duplicated code, violating the DRY principle.
  • Error codes can overlap with legitimate return values, making it hard to indicate when an actual error has occurred. This is known as the semipredicate problem.
  • Sometimes things happen that are so catastrophic, you don’t even bother with a strategy for trying to cope with them. For example, it might be perfectly acceptable to die if malloc() fails, unless you’re fully equipped to keep your program running when the machine is out of memory.

Languages like .NET are free from these worries because they use a different strategy: assume everything will always succeed (try), and handle any problems later in one single location (catch).

public void Foo(){    try    {        DoSomething();        DoSomethingElse();        DoThirdThing();    }    catch    {        // bail out    }}

This lets you focus on the success case (the one that actually matters), instead of cluttering your code up with error handling.

Unfortunately, it seems a lot of .NET developers only think of exceptions as things you need to catch when calling the Base Class Library, and shy away from throwing and catching their own exceptions. Instead, I see kludges like:

if (!DoSomething())   // bail out

Or even this one, which simulates Exception.Message:

string errorMessage = DoSomething();if (errorMessage != "")   // bail out

The golden rule is, if your method encounters a condition that prevents it from achieving its intended purpose, it should throw an exception.

Re-throwing exceptions

Do you know the difference between the two following code snippets?

catch (Exception ex){        // log the exception        ...                // re-throw        throw ex;}catch (Exception ex){        // log the exception        ...                // re-throw        throw;}

The first example resets ex’s stack trace to the current location. This is fine when you’re throwing a brand new exception, but not so good if you’re just passing one along — you’ll lose the stack trace telling you where it originated from.

To preserve the stack trace, just use “throw” by itself with no argument, as in the second example.

Use parameterless catch/throw if you don’t care about the details

You don’t have to specify any parameter name in your catch block if you don’t need the exception object itself:

catch (SqlException){        // rollback transaction        // rethrow        throw;}

This is handy for eliminating that “variable ‘ex’ is not used” compiler warning. In fact, if you don’t care about the type, you can get rid of the brackets altogether:

catch // anything{        // handle the exception}

But be careful with one — in 99% of situations, we do care what the type is.

Only catch exceptions you can handle

Imagine you are working on some code that tries to parse some user input as an integer. If it’s invalid (e.g. not a number), we’ll display an error message.

int value = 0;try{        // forget about TryParse() for the moment :)        value = Int32.Parse(someUserInput);}catch (Exception ex){        // invalid input}

What’s wrong with this? We’ve specified that this catch block is capable of handling any type of exception, no matter how severe or unrelated. They’ll all be interpreted as validation errors — OutOfMemoryException, ThreadAbortException — even NullReferenceException (which might indicate an actual bug in the code).

In reality, however, all we really care about is a very narrow subset of exceptions: those that indicate the number couldn’t be parsed correctly.

int value = 0;try{        value = Int32.Parse(someUserInput);}catch (FormatException ex){        // invalid input}

Only catch exceptions you anticipate could be thrown as a consequence of the code within your try block. Don’t try to handle anything outside that window, unless you have a specific strategy to deal with it.

Multiple catch blocks

Here’s an example of some code I saw at my last job, that did different things depending on the type of exception:

try{        ...}catch (Exception ex){        if (ex is FileNotFoundException)                // do stuff        else if (ex is IOException)                // do stuff        else                // ???}

Yuck — using reflection and a big if-statement to differentiate types is generally a sign of bad code in .NET (or any OO language), and catch blocks are no exception (ba-dum-psh). Instead, use multiple catch blocks with overloads for different exception types. At runtime, the .NET framework will call the first matching catch block, so you need to specify them in most-to-least specific order:

try{        ...}catch (FileNotFoundException ex){        // file not found (subclass of IOException)}catch (IOException ex){        // some other file-related error}

Inner exceptions

In .NET, low-level exceptions can be wrapped up into a higher-level context that makes more sense in the grand scheme of things. For example, a CouldNotOpenDocumentException might be caused by a FileNotFoundException. This is reflected in the Exception.InnerException property, and inner exception each is complete with its own error message and stack trace.

Here’s some code I saw once for unwrapping them, via a loop:

try{        ...}catch (Exception ex){        // get exception details        string errMessage = ex.Message + ex.StackTrace;        Exception innerException = ex.InnerException;        while (innerException != null)        {                errMessage += innerException.StackTrace;                innerException = innerException.InnerException;        }        // log errMessage}

This code is pretty redundant (and ugly), as the .NET framework will do this for you. Exception.ToString() will print out the exception’s message and stack trace, then call itself on the inner exception:

try{        ...}catch (Exception ex){        // get full stack trace        string errMessage = ex.ToString();        // log errMessage}

This will return a complete dump of the inner exception tree, producing a message like:

Could not get OSM Status menu item status. ---> System.NullReferenceException: Object reference not set to an instance of an object.   at SMS.Common.GetOSMMenuStatus(Space oSpace) in D:DevXYZSourceXYZ.WebsiteCodeCommonCommon.cs:line 1051   --- End of inner exception stack trace ---   at SMS.Common.GetOSMMenuStatus(Space oSpace) in D:DevXYZSourceXYZ.WebsiteCodeCommonCommon.cs:line 1058   at SMS.Common.AppendSMSNav(Space& oSpace, DateTime& dtSMSDate, DateTime& dtStartDate, DateTime& dtEndDate) in D:DevXYZSourceXYZ.WebsiteCodeCommonCommon.cs:line 837

Another tip for is finding the root inner exception — the original cause. The developers from the previous example chose to drill through InnerExceptions in a loop until they reached the bottom (null). An easier way would just be to call Exception.GetBaseException().

Defensive coding

To write robust and idiot-proof code, you have to assume people are going to try to break it. This means checking input parameters are valid at the start of every method.

You should continue this habit all throughout internal application methods as well, so bad data gets stopped short at the point of origin, instead of trickling through and causing problems later on.

public class ProjectsController : Controller{    IProjectRepository projectRepository;        public ProjectsController(IProjectRepository projectRepository)    {        // better to have an exception here, when the controller is constructed...        if (projectRepository == null)            throw new ArgumentNullException("projectRepository");        this.projectRepository = projectRepository;    }    public ActionResult Detail(string name)    {        // ...than down here, when an action gets called.        Project p = this.projectRepository.GetByName(name);        return View(p);    }    ...}

Note that Microsoft’s Spec# contracts will make these sorts of checks much easier in future, with built-in syntax for not-nullable parameters:

public ProjectsController(IProjectRepository! projectRepository){    // method will not be entered if projectRepository is null    this.projectRepository = projectRepository;}

Plus these rules are enforced at compile time as well! So the following line would not build:

ProjectsController controller = new ProjectsController(null); // error

See Microsoft’s article on Best Practices for Handling Exceptions for more .NET exception tips.

Find missing foreign/primary keys in SQL Server

Find missing foreign/primary keys in SQL Server

Last week I wrote a SQL query to estimate how many columns are missing from foreign or primary keys. This works because of our naming convention for database keys:

  • We use a Code suffix for natural keys e.g. CountryCode = NZ
  • We use an ID suffix for surrogate keys e.g. EmployeeID = 32491

This script looks for any columns that match this naming pattern, but aren’t part of a primary or foreign key relationship.

-- Find columns on tables with names like FooID or FooCode which should-- be part of primary or foreign keys, but aren't.SELECT        t.name AS [Table],        c.name AS [Column]FROM        sys.tables t        INNER JOIN sys.syscolumns c ON                c.id = t.object_id                -- Join on foreign key columns        LEFT JOIN sys.foreign_key_columns fkc ON                (fkc.parent_object_id = t.object_id                AND c.colid = fkc.parent_column_id)                OR (fkc.referenced_object_id = t.object_id                AND c.colid = fkc.referenced_column_id)                -- Join on primary key columns        LEFT JOIN sys.indexes i ON                i.object_id = t.object_id                and i.is_primary_key = 1        LEFT JOIN sys.index_columns ic ON                ic.object_id = t.object_id                AND ic.index_id = i.index_id                AND ic.column_id = c.colidWHERE        t.is_ms_shipped = 0        AND (c.name LIKE '%ID' OR c.name LIKE '%Code')        AND        (                fkc.constraint_object_id IS NULL -- Not part of a foreign key                 AND ic.object_id IS NULL -- Not part of a primary key        )        AND        (                -- Ignore some tables                t.name != 'sysdiagrams'                AND t.name NOT LIKE '[_]%' -- temp tables                AND t.name NOT LIKE '%temp%'                AND t.name NOT LIKE '%Log%' -- log tables                                -- Ignore some columns                AND c.name NOT IN ('GLCode', 'EID', 'AID') -- external keys        )ORDER BY        t.name,        c.name

Using this script, I found over 200 missing foreign keys in one production database!

Back to basics: good source control check-in habits

Back to basics: good source control check-in habits

The other day at work I went over a few good source control habits (TFS-centric) for new developers, and why they’re worth doing. Here are some tips:

Check-ins are coarse-grained

When I first started using source control, a lot of my check-in logs looked like this:

  1. Added useful function foo() (that will be ultimately required for feature X)
  2. Added feature X

I was trying to be helpful by making foo() available for use as early as possible, but most of the time I checked it in too early, and needed to change/fix it later — resulting in additional check-ins. In reality though, all this did was clutter up the change history with signal-to-noise, making it harder to establish what changes were actually required to effect a change.

It also of course violates the YAGNI (you ain’t gonna need it) principle – no one ever needed foo() badly enough to make it worth checking it in early.

Check-ins should be coarsely grained — instead of staggering code changes over a period of time, save it all for one big changeset that contains everything. This actually improves YAGNI — instead of adding endless low-level functions ‘because they might be useful’, you’re forced to associate them with a higher application-level change.

Never check in unfinished work

In the main (trunk) branch, there are two cardinal rules:

  • Never check in anything that breaks the build
  • Never check in an unfinished feature

The main branch should always be in a state where it’s ready for release. Features have been entirely added or don’t exist yet; complete changes have been made, or not even started. There is no middle ground.

Having to worry about the possibly unfinished state of the ‘master copy’ is just another burden on your team. Even worse, if you check in code that doesn’t compile, you can interrupt other people’s work and waste their time commenting out bad blocks and working around it.

Sometimes, however, you’ll get a piece of work that doesn’t fit in one check-in.

  • It could take a long time to implement (needs more than one check-in).
  • Multiple developers might need to work on it.
  • The change isn’t going to be released (committed to the main branch) until a later date.

You have two options: fork a copy of the code for a new development branch, or shelve your changes so you or someone else can work on them later. The basic rule for branches is if you need more than one check-in to make a change, you need to branch and merge back in later.

Source control is not for back ups

This follows from the previous two tips. Check in your work because you’re completely done and finished — not because it’s the end of the day, or because it’s the weekend etc.

I think this habit often begins when using reserved-checkout source control systems like VSS, where having a file checked out explicitly blocks other people from editing it. Everyone who’s used VSS knows from experience that hilarious situation where you need to edit a checked-out file, but the person who has it is on holiday or otherwise unreachable.

VSS file exclusively checked out error

To avoid this, teams adapt their work habits to suit VSS, checking in frequently so as not to lock files for extended periods. This practice is continued for some reason (cargo cult programming?) when moving to less brain-damaged source control tools like TFS or subversion.

Anyway, if you want nightly backups of your local working source code, get some backup software. Don’t use source control — change tracking becomes difficult when you check in based on calendar events. It gets even more ridiculous if you have changes that don’t compile yet, and have to comment/uncomment code when checking it in.

Use a separate check-in when reformatting code

Say you go to edit a file, but the formatting is all messed up — the indenting is wrong. So you fix it up while making your changes. But then, if someone comes along later to see what your change entailed, a diff tool will highlight differences on every line, making it very hard to see what the actual code change was. Instead, use a separate check-in for mass code reformatting.

Write useful check-in comments

If you’re looking over the history of a file in source control, comments like ‘css fix’ or ‘#12345′ won’t be very useful. Instead, good check-in comments need three things:

  • What was changed
  • Why it was changed
  • Any bug/issue tracking numbers (if applicable)

Describe what you changed, and why it was necessary. You don’t have to write an essay — I find one sentence of ‘did X because Y’ is usually sufficient.

TFS change history

You should also list any bug/issue tracking numbers, or make reference to any relevant project documentation (e.g. list of requirements) to provide a higher-level context. This is a good way of establishing traceability on tightly-controlled source repositories — if you do it right, you’ll find almost every change can be traceable back to some external identifier.

I also like to paste my changeset number when I update an issue as being completed (ready for user testing or peer review) — the easier it is to find my changes, the better! (Note that some source control tools can do this automatically from your check-in comment).

Real-life examples

You can see good check-in habits in action in most popular open source projects. Here are a few of examples:

  • Boost C++ Libraries revision log
  • Ruby on Rails commit history
  • Subtext revision log

If it ain’t broke, don’t fix it

If it ain’t broke, don’t fix it

Resisting the urge to rewrite other peoples’ code is one of the hardest challenges an inexperienced developer may encounter when working in a team environment. It is perfectly understandable; after all it is much easier to write code than to understand it. Not to mention the desire to fix and improve on any genuine flaws that may exist.

Recently, I have been involved in a number of projects that require extending and changing existing applications, developed by other people. Many times I have been tempted to rewrite functions that look buggy or inefficient. Much of the code is simply downright awful. However, as bad as it may seem to me, it works and it is trusted. I cannot touch it. In the increasingly marshaled enterprise environment I work in, changes that have not been agreed with the customer simply cannot be made. System requirements have formed a project contract which I cannot step outside of.

Existing code has been tested. Other people understand it. It is known to work. Making unnecessary changes risks introducing new bugs. Introducing a new bug would be a big step backwards.

Ultimately, because you’re not adding anything new, any unnecessary changes will be a complete waste of time, in the eyes of your customer. And if you’re being paid for your work, a waste of time equates to a waste of money.

No matter how bad the code, no matter how brain-damaged the design, do not change anything unless you absolutely have to. Log issues. Discuss them. But otherwise, if it ain’t broke, don’t fix it!