HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Even handler for text input preview

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
previewtextinputforevenhandler

Problem

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.InvariantCultureIg

Solution

-
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;

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

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

-
Try to use LINQ where possible.

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

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

-
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.

-
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.

Code Snippets

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;
}

Context

StackExchange Code Review Q#5754, answer score: 7

Revisions (0)

No revisions yet.