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!

StringWalker class:

public class StringWalker
{
	string _s;
	public int Index { get; set; }

	public StringWalker(string s)
	{
		_s = s;
		Index = -1;
	}

	public bool MoveNext()
	{
		if (Index == _s.Length - 1)
			return false;
		Index++;
		return true;
	}

	public char CharAtIndex()
	{
		return _s[Index];
	}
};

IndentWriter class:

public class IndentWriter
{
	StringBuilder _sb = new StringBuilder();
	int _indent;

	public void Indent()
	{
		_indent++;
	}

	public void UnIndent()
	{
		if (_indent > 0)
			_indent--;
	}

	public void WriteLine(string line)
	{
		_sb.AppendLine(CreateIndent() + line);
	}

	private string CreateIndent()
	{
		StringBuilder indentString = new StringBuilder();
		for(int i = 0; i< _indent; i++)
			indentString.Append("    ");
		return indentString.ToString();
	}

	public override string ToString()
	{
		return _sb.ToString();
	}
};

JSON formatter class:

public class JsonFormatter
{
	StringWalker _walker;
	IndentWriter _writer = new IndentWriter();
	StringBuilder _currentLine = new StringBuilder();
	bool _quoted;

	public JsonFormatter(string json)
	{
		_walker = new StringWalker(json);
		ResetLine();
	}

	public void ResetLine()
	{
		_currentLine.Length = 0;
	}

	public string Format()
	{
		while(MoveNextChar())
		{
			if (this._quoted == false && this.IsOpenBracket())
			{
				this.WriteCurrentLine();
				this.AddCharToLine();
				this.WriteCurrentLine();
				_writer.Indent();
			}
			else if (this._quoted == false && this.IsCloseBracket())
			{
				this.WriteCurrentLine();
				_writer.UnIndent();
				this.AddCharToLine();
			}
			else if (this._quoted == false && this.IsColon())
			{
				this.AddCharToLine();
				this.WriteCurrentLine();
			}
			else
			{
				AddCharToLine();
			}
		}
		this.WriteCurrentLine();
		return _writer.ToString();
	}

	private bool MoveNextChar()
	{
		bool success = _walker.MoveNext();
		if (this.IsApostrophe())
		{
			this._quoted = !_quoted;
		}
		return success;
	}

	public bool IsApostrophe()
	{
		return this._walker.CharAtIndex() == '"';
	}

	public bool IsOpenBracket()
	{
		return this._walker.CharAtIndex() == '{'
			|| this._walker.CharAtIndex() == '[';
	}

	public bool IsCloseBracket()
	{
		return this._walker.CharAtIndex() == '}'
			|| this._walker.CharAtIndex() == ']';
	}

	public bool IsColon()
	{
		return this._walker.CharAtIndex() == ',';
	}

	private void AddCharToLine()
	{
		this._currentLine.Append(_walker.CharAtIndex());
	}

	private void WriteCurrentLine()
	{
		string line = this._currentLine.ToString().Trim();
		if (line.Length > 0)
		{
			_writer.WriteLine(line);
		}
		this.ResetLine();
	}
};

Few samples:

Console.WriteLine(new JsonFormatter(
	@"{""parameter"" : ""value"" , { ""parameter2"" : ""value2"" },{ ""parameter3"" : ""value3"" } }").Format());
Console.WriteLine(new JsonFormatter(
	@"{""parameter"":[""value1"",""value2"",""value3""] }").Format());
Console.WriteLine(new JsonFormatter(
	@"{""parameter"": ""value with {brackets}"" }").Format());

…and results:

{
    "parameter" : "value" ,
    {
        "parameter2" : "value2"
    },
    {
        "parameter3" : "value3"
    }
}

{
    "parameter":
    [
        "value1",
        "value2",
        "value3"
    ]
}

{
    "parameter": "value with {brackets}"
}

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, compile-time checked INotifyPropertyChanged implementation.