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: 'Lesnikowski.Client.Pop3.MessageCount' is obsolete: 'Please use the return value of GetAccountStat method instead.'

warning CS0618: 'Lesnikowski.Client.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!

There is a bug in .NET 2.0 regarding timeout handling by Socket class.
Here’s the code that reproduces this behavior.

using(TcpClient client = new TcpClient())
{
    // Set timeout
    client.ReceiveTimeout =
        (int)TimeSpan.FromSeconds(1).TotalMilliseconds;

    // Connect to perfectly working imap server
    client.Connect("mail.lesnikowski.com", 143);

    // Create reader & writer
    NetworkStream stream = client.GetStream();
    StreamReader reader = new StreamReader(stream);
    StreamWriter writer = new StreamWriter(stream);

    // Read hello line
    Console.WriteLine(reader.ReadLine());

    // this works with no problems
    writer.WriteLine("a NOOP");             // No operation command
    writer.Flush();
    Console.WriteLine(reader.ReadLine());   // No operation response

    try
    {
        reader.ReadLine();                  // Nothing to read
    }
    catch (Exception ex)                    // Timeout
    {
        Console.WriteLine("timeout");       // This is expected
    }

    writer.WriteLine("b NOOP");             // No operation command
    writer.Flush();

    // SocketException: A non-blocking socket operation
    // could not be completed immediately
    Console.WriteLine(reader.ReadLine());   

    client.Close();
}

The above code runs as expected on Mono and on .NET 4.0.

The problem is in UpdateStatusAfterSocketError internal Socket method that executes SetToDisconnected method when any exception is thrown, including timeout exception.

Recently we had a following problem with NHibernate, and although I love NHiberante, it does not always behave as expected.

We have a Person class correctly mapped in NHibernate:

public class Person
{
	public int Id { get; set; }
	public int Name { get; set; }
}

We saw that sometimes we were getting a Person with Id equal to zero, from our repository:

public class PersonRepository
{
	private ISession _session;

	//...

	public Person LoadById(int id)
	{
		return _session.Load<Person>(id);
	}
}

We narrowed the problem down and wrote this little test:

[Test]
public void LoadById_Loads_IdIsSet()
{
    _context.ExecuteInTransaction(() =>
		{
			Person person = _context.PersonRepository.LoadById(7);
			Assert.AreEqual(7, person.Id);
		}
	);
}

…which of course failed.

After initial surprise, we took a second look at the Person class and we saw that we are missing virtual keyword on properties. NHibernate is not able to create a correct Proxy object.

public class Person
{
	public virtual int Id { get; set; }
	public virtual int Name { get; set; }
}

This fixed the issue.
Strange thing is that we expect that NHibernate would throw an exception is such case.

There are several ways of implementing INotifyPropertyChanged in WPF applications.

One that I particularly like is by using PostSharp to implement INotifyPropertyChanged.

If you are not comfortable with PostSharp you can at least get rid of those nasty strings in standard implementation:

public class Model : ViewModeBase
{
    private string _status;
    public string Status
    {
        get { return _status; }
        set
        {
            _status = value;
            OnPropertyChanged("Status");
        }
    }
};

Labda expressions look nicer and are easier to refactor:

public class MyModel : ViewModeBase
{
    private string _status;
    public string Status
    {
        get { return _status; }
        set
        {
            _status = value;
            OnPropertyChanged(() => Status);
        }
    }
};

First thing we need to do is to create base class for all our ViewModels:

public class ViewModelBase
{
    public event PropertyChangedEventHandler PropertyChanged
        = delegate { };

    protected void OnPropertyChanged(
        Expression<Func<object>> expression)
    {
        string propertyName = PropertyName.For(expression);
        this.PropertyChanged(
            this,
            new PropertyChangedEventArgs(propertyName));
    }
};

The implementation of PropertyName.For is very straightforward: How to get property name from lambda.

That’s it!
Nice, easy to refactor, compilaile-time checked INotifyPropertyChanged implementation.

The whole idea is simple, we want this test to pass:

[Test]
void CanGetPropertyName_UsingLambda()
{
  Assert.AreEqual("Name", PropertyName.For<Person>(x => x.Name));
}

It seams nice and convenient way of getting property name.
Whole test fixture looks as follows:

[TestFixture]
public class PropertyNameTests
{
    public string NameForTest { get; set; }

    [Test]
    public void CanGetPropertyName_SameType_UsingLambda()
    {
        Assert.AreEqual("NameForTest",
            PropertyName.For(() => NameForTest));
    }

    [Test]
    public void CanGetPropertyName_UsingLambda()
    {
        Assert.AreEqual("Name",
            PropertyName.For<Person>(x => x.Name));
        Assert.AreEqual("Age",
            PropertyName.For<Person>(x => x.Age));
    }

    [Test]
    public void CanGetPropertyName_Composite_UsingLambda()
    {
        Assert.AreEqual("Home.City",
            PropertyName.For<Person>(x => x.Home.City));
        Assert.AreEqual("Home.FlatNumber",
            PropertyName.For<Person>(x => x.Home.FlatNumber));
    }
}

public class Person
{
    public string Name { get; set; }
    public int Age { get; set; }
    public Home Home { get; set; }
}

public class Home
{
    public string City { get; set; }
    public string FlatNumber { get; set; }
}

Implementation uses .NET 3.5 feature called expression trees. Expression trees represent language-level code in the form of data. The data is stored in a tree-shaped structure.

/// <summary>
/// Gets property name using lambda expressions.
/// </summary>
internal class PropertyName
{
    public static string For<T>(
        Expression<Func<T, object>> expression)
    {
        Expression body = expression.Body;
        return GetMemberName(body);
    }

    public static string For(
        Expression<Func<object>> expression)
    {
        Expression body = expression.Body;
        return GetMemberName(body);
    }

    public static string GetMemberName(
        Expression expression)
    {
        if (expression is MemberExpression)
        {
            var memberExpression = (MemberExpression)expression;

            if (memberExpression.Expression.NodeType ==
                ExpressionType.MemberAccess)
            {
                return GetMemberName(memberExpression.Expression)
                    + "."
                    + memberExpression.Member.Name;
            }
            return memberExpression.Member.Name;
        }

        if (expression is UnaryExpression)
        {
            var unaryExpression = (UnaryExpression)expression;

            if (unaryExpression.NodeType != ExpressionType.Convert)
                throw new Exception(string.Format(
                    "Cannot interpret member from {0}",
                    expression));

            return GetMemberName(unaryExpression.Operand);
        }

        throw new Exception(string.Format(
            "Could not determine member from {0}",
            expression));
    }
}

UnaryExpression part is needed for value types to work.