on building software and distributed team culture
I’m not going to do this often but I just found out who said one sentence that I just adore and I wanted to store it here for further reference.
As far as the customer is concerned, the Interface is the product.
This is so true, and yet too often user experience is so low on project’s priority list. There’s another quote on this:
Users do not care about what is inside the box, as long as the box does what they need done.
Jef Raskin
First of all, take a look at the following code:
private string _targetText;
private int _maxLines;
private int _maxSize;
public int Lines
{
get
{
if (_targetText == null)
return 0;
return _targetText.Split('\n').Length;
}
}
public bool IsValid
{
get
{
return _maxSize == 0 ?
true :
Size <= _maxSize
&&
_maxLines == 0 ?
true :
Lines <= _maxLines;
}
}
It’s fairly simple, the most important piece is IsValid property, that checks if _targetText meets certain length and number of lines limitations.
Now, let’s say that max size is 5, max lines is 1 and target text is “Some incredibly long piece of text“. Million dollar question is: What would IsValid return for these parameters?
It would actually return true, because there is a subtle bug in this code. It may not be apparent and and it’s a tricky beast because you have to know how to look at it to see what’s actually going on. The reason why it returns true, when all signs on earth and in heaven say it should return false is operators priority, and the way how ?: gets translated by compiler to some other code.
Logically thinking we would expect the code to examine if maxSize is 0 and if it is to set left hand flag to true, and if it’s not zero to set it to whether or not Size is less or equal max size, then to do similar thing with maxLines and Lines and set right hand flag, and then, if both flags are true to return true, and false otherwise. By thinking this way we assume that it will first run both ?: operators and then && the results, in other words, we assume that ?: operator has higher priority than && operator that turns not to be true.
That’s because people think of ?: operator like shorthand of if else, whereas mighty Reflector reveals its true nature to be different. When we compile code above and then open it in Reflector we’ll see code like this (line breaks and indents added to make it easier to read).
public bool IsValid
{
get
{
return
(
(this._maxSize == 0)
||
(
(
(this.Size <= this._maxSize)
&&
(this._maxLines == 0)
)
||
(this.Lines <= this._maxLines)
)
);
}
}
I suspect that this code looks slightly different than what you expected. No if else only logical ands and ors. And it’s the reason for that unexpected output. If you examine that code closely you’ll notice that no mater size and max size – if number of lines is not greater than maxLines it will return true. So how to fix that code? Either by surrounding ?: operators in brackets, or by moving them to other properties/methods like this:
public bool IsValid
{
get
{
return HasValidSize
&& HasValidLineCount;
}
}
public bool HasValidLineCount
{
get
{
return _maxLines == 0 ?
true :
Lines <= _maxLines;
}
}
public bool HasValidSize
{
get
{
return _maxSize == 0 ?
true :
Size <= _maxSize;
}
}
I hope that was informative.
How often do you find yourself writing code like this:
string[] files = GetFiles(path);
filesListView.BeginUpdate();
for (int i = 0; i < files.Length; i++)
{
//possibly something more
filesListView.Items.Add(new ListViewItem(files[i]));
}
filesListView.EndUpdate();
You suspend control with BeginUpdate() in order not to repaint itself with every change you make, then do several updates to it, and then you let go of it, with EndUpdate(), so that it could repaint itself to reflect all the changes. Easy and simple isn’t it? So, what’s the problem? I can see at least two:
I created only very few lines of code for this example, but you have to look into the code to find the portion between BeginUpdate, and EndUpdate. It’s not apparent at the first glimpse of an eye, as it should be. I think that you should be able to immediatelly tell, what portion of the code happens when the control is suspended, and what not. With this code, you first have to parse it to find BeginUpdate(), and then matching EndUpdate.
And hey, have you ever forgotten to add EndUpdate, at the end of similar code snippet? It, compiles, you don’t even get a warning, but it works far from what you would like it to.
To overcome those constraints I decided to create a simple helper class that would allow me to change code above to something like:
string[] files = GetFiles(path);
using (new Update<ListView>(filesListView))
{
for (int i = 0; i < files.Length; i++)
{
//possibly something more
filesListView.Items.Add(new ListViewItem(files[i]));
}
}
//do something else if you need to
Isn’t it much cleaner? Immediately from looking at the code you can see which portion is in Update block. You also don’t have to worry
that you forget to call EndUpdate() – it will be done for you.
If you ever played with WinForms controls, you certainly noticed that not every control has Begin/EndUpdate pair of methods. I never actually checked, but I assumed that for those who do have them, those methods come from some base class, or maybe interface. I was greatly surprised to discover, that neither of those assumptions was true. There simply are controls that do have those methods, and those that do not, period. I guess that’s not the best solution, and there should be and interface like IUpdateable, ISupportsUpdate or something.
Lack of that interface made me use Update<T> where T:Control instead of Update<T> where T:IUpdateable, so now you can pass in any control, even one, that has no Begin/EndUpdate methods implemented.
So what now? Throw NotImplementedException? That was my initial thought, but it means that everything would compile, and you would get exception during runtime which is something I really dislike. This leaves us with another option: if given control doesn’t implement said methods do nothing. It may seem the most reasonable option but it promises consistent behavior, and doesn’t provide it.
Imagine code like this:
using (new Update<Label>(myLabel))
{
for (int i = 0; i < 10; i++)
{
myLabel.Text += i.ToString();
Application.DoEvents();
Thread.Sleep(500);
}
}
Label, obviously doesn’t have Begin/EndUpdate, but it’s a Control and this code would compile. Someone would expect that for 5 seconds label’s text stays unchanged, and after that it updates, all the changes in one go. Well, he/she would be bitterly disappointed by the outcome, since instead of one change, we would have 10 of them. It’s not what we wanted, it’s not what we ought to have, since it means that we get inconsistent behavior.
So what now? I poke around a little bit with Reflector, and I discovered, that Control class has internal method BeginUpdateInternal, and consequently EndUpdateInternal. That’s what we wanted! Althought it’s a hacks, because we need to use reflection to call internal method from outside of its assembly it’s the best option we have in this situation. Now we can call public methods for controls that have them, and Control’s internal classes for other ones.
Still, I’m not completely satisfied with this solution. I shouldn’t be able to do it for Label, TextBox or any other control that is not meant to be updated this way, but to do this, all those controls that do support this behavior should all be marked with Interface, and that’s something that only BCL team can do.
Final code for Update<T> looks like this:
public class Update<T> : IDisposable where T : Control
{
private T _control;
private Type _type;
private bool _hasPublicUpdate = false;
public Update(T control)
{
_type = control.GetType();
MethodInfo beginUpdate = _type.GetMethod("BeginUpdate");
if (beginUpdate == null)
{
_type = typeof(System.Windows.Forms.Control);
beginUpdate = _type.GetMethod("BeginUpdateInternal", BindingFlags.NonPublic | BindingFlags.Instance);
}
else
{
_hasPublicUpdate = true;
}
_control = control;
beginUpdate.Invoke(_control, null);
}
public void Dispose()
{
MethodInfo endUpdate = null;
if (_hasPublicUpdate)
{
endUpdate = _type.GetMethod("EndUpdate");
endUpdate.Invoke(_control, null);
}
else
{
endUpdate = _type.GetMethod("EndUpdateInternal", BindingFlags.NonPublic | BindingFlags.Instance, null, new Type[] { typeof(bool) }, new ParameterModifier[] { new ParameterModifier(1) });
endUpdate.Invoke(_control, new object[] { true });
}
_control = null;
GC.SuppressFinalize(this);
}
}