Better public API: GetAccountStat

Here’s some code I recently found in Mail.dll and decided to refactor.

// Before:

using(Pop3 client = new Pop3())
{
    client.GetAccountStat();
    Console.WriteLine(
        "Inbox has {0} emails.",
        client.MessageCount);
}

There are several things wrong with this code.

• The method is called Get… but it does not get anything, it changes the internal state of the object.
• Message count is stored in Pop3 object:
  • If the user of your API connects later to a different server you need to remember to reset this variable.
  • If the user of your API forgets to call GetAccountStat, message count is undefined (Should it be null?)

It’s really hard to say that this is a friendly API, as it requires the user to perform actions in specific order (call GetAccountStat before accessing message count).

Another problem is that GetAccountStat method is responsible for parsing the server response. It’s not necessary a bad thing, but if you have hundreds such methods then Pop3 class gets bloated with hard-to-test parsing logic.

Now lets take a look at the After code:

// After:

using(Pop3 client = new Pop3())
{
    AccountStats stats = client.GetAccountStat();
    Console.WriteLine(
        "Inbox has {0} emails.",
        stats.MessageCount);

}

Here we can see a good API:

• Method is called Get…. and it actually gets something.
• No specific call order is required, you simply call one method and act on the result.
• Parsing logic was moved to the AccountStats class.

This is not seen here but AccountStats method has a static Parse method…and look how easy is to write unit test for it’s behavior:

[Test]
public void Parse_MessageCountAndMailboxSize_AreFilled()
{
    AccountStats stats = AccountStats.Parse("2 373766");
    Assert.AreEqual(2, stats.MessageCount);
    Assert.AreEqual(373766, stats.MailboxSize);
}

Note also that actually we have NOT introduced a breaking change to our public API. Following code still works:

using(Pop3 client = new Pop3())
{
    client.GetAccountStat();
    Console.WriteLine(
        "Inbox has {0} emails.",
        client.MessageCount);
}

You’ll get 2 obsolete warnings:

warning CS0618: 'Limilabs.Client.POP3.Pop3.MessageCount' is obsolete: 'Please use the return value of GetAccountStat method instead.'

warning CS0618: 'Limilabs.Client.POP3.Pop3.MailboxSize' is obsolete: 'Please use the return value of GetAccountStat method instead.'

As we marked MessageCount and MailboxSize with [obsolete] attribute, but that’s it!

Tags: Clean code