Even handler for text input preview

Question

In a WPF application I have the following event handler for PreviewTextInput event in ComboBox control. The main purpose of this code is to select an item in ComboBox via pressing a letter key. There is duplicated logic in this code and I want it removed.

private void OnTextComboBoxPreviewTextInput(object sender, TextCompositionEventArgs e)
{
 var comboBox = sender as ComboBox;
 if (!comboBox.IsDropDownOpen)
 {
 return;
 }
 if (!comboBox.IsEditable || !comboBox.IsReadOnly)
 {
 return;
 }
 foreach (var item in comboBox.Items)
 {
 if (item == null)
 {
 continue;
 }
 var textSearch = TextSearch.GetTextPath(comboBox);
 var stringItem = item as string;
 if (stringItem != null)
 {
 if (stringItem.StartsWith(e.Text, StringComparison.InvariantCultureIgnoreCase))
 {
 SelectItemInComboBox(comboBox, item);
 break;
 }
 continue;
 }
 var dependencyObjItem = item as DependencyObject;
 if (dependencyObjItem != null)
 {
 var textMember = TextSearch.GetText(dependencyObjItem);
 if (!string.IsNullOrEmpty(textMember))
 {
 var selectorFunc = ExpressionHelper.GetMemberFunction(dependencyObjItem, textMember);
 var textValue = selectorFunc(dependencyObjItem);
 if (textValue.ToString().StartsWith(e.Text, StringComparison.InvariantCultureIgnoreCase))
 {
 SelectItemInComboBox(comboBox, item);
 break;
 }
 continue;
 }
 }
 if (!string.IsNullOrEmpty(textSearch))
 {
 var selectorFunc = ExpressionHelper.GetMemberFunction(item, textSearch);
 var textValue = selectorFunc(item);
 if (textValue.ToString().StartsWith(e.Text, StringComparison.InvariantCultureIgnoreCase))
 {
 SelectItemInComboBox(comboBox, item);
 break;
 }
 }
 }
 e.Handled = true;
}
private void SelectItemInComboBox(ComboBox comboBox, object item)
{
 var comboBoxItem = comboBox.ItemContainerGenerator.ContainerFromItem(item) as ComboBoxItem;
 comboBoxItem.IsSelected = true;
}

Answer 1

1. I prefer a simple cast over as when I don't expect an object to be of any other type. If you do expect this, use as and be sure to check for null.

   var comboBox = (ComboBox)sender;

2. When it makes sense to group code together do so.

   if (!comboBox.IsDropDownOpen || !combobox.IsEditable || !combobox.IsReadOnly)

3. Try to use LINQ where possible.

4. Place variables close to where they are being used. (textSearch)

5. Don't overuse var. This is subjective, and there are several discussion about it. (also see comments in the code underneath)

6. Using continue can make it more difficult to get an overview of the code. In some occasions it is useful, but look at the code below to see how you could replace it with normal if else conditions instead.

7. Identify redundant code, see what you can eliminate and what you can't. You might have to introduce new intermediate variables. (e.g. textToCompare)

Here is the complete reworked example:

private void OnTextComboBoxPreviewTextInput(object sender, TextCompositionEventArgs e)
{
 var comboBox = (ComboBox)sender;
 if (!comboBox.IsDropDownOpen || !combobox.IsEditable || !combobox.IsReadOnly)
 {
 return;
 }
 foreach (var item in comboBox.Items.Where( i => i != null ))
 { 
 string textToCompare = null;
 if (item is string)
 {
 textToCompare = (string)item;
 }
 else if (item is DependencyObject)
 {
 DependencyObject dependencyObjItem = (DependencyObject)item;
 string textMember = TextSearch.GetText(dependencyObjItem);
 if (!string.IsNullOrEmpty(textMember))
 {
 // From this sample, I don't know what the following type is!
 // I wouldn't use var here.
 var selectorFunc = ExpressionHelper.GetMemberFunction(dependencyObjItem, textMember);
 textToCompare = selectorFunc(dependencyObjItem);
 }
 }
 else
 {
 string textSearch = TextSearch.GetTextPath(comboBox);
 if (!string.IsNullOrEmpty(textSearch))
 {
 var selectorFunc = ExpressionHelper.GetMemberFunction(item, textSearch);
 textToCompare = selectorFunc(item);
 }
 }
 if (textToCompare != null && textToCompare.StartsWith(e.Text, StringComparison.InvariantCultureIgnoreCase))
 {
 SelectItemInComboBox(comboBox, item);
 break;
 }
 }
 e.Handled = true;
}
private void SelectItemInComboBox(ComboBox comboBox, object item)
{
 var comboBoxItem = comboBox.ItemContainerGenerator.ContainerFromItem(item) as ComboBoxItem;
 comboBoxItem.IsSelected = true;
}

I will leave it up to you as an exercise whether you can further eliminate the duplicate code from selectorFunc, and whether it would be useful.

• Steven Jeuris: break could of course be replaced with a while statement. But in this case I don't see the problem with it as the break statement is near the end of the for, clearly indicating an early out.
• Steven Jeuris: As a minor subtle (again subjective) side note. If SelectItemInComboBox is only called from one place, I wouldn't separate it in a function.
• Sergey K: @Steven Jeuris Thank you for good answer, I can answer for you comment in code // From this sample, I don't know what the following type is!//I wouldn't use var here var selectorFunc - will be a type of Func<object,object>
• Sergey K: and this var selectorFunc = ExpressionHelper.GetMemberFunction(item, textSearch); textToCompare = selectorFunc(item); I've extracted in method to remove duplication .