Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite

Hello every one in the WPF application I have the following event handler for PreviewTextInput event in ComboBox control.

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

The main purpose of this code is to select an item in ComboBox via pressing letter key. in this code there are duplication logic. Help me please to refactor this code for remove duplication. Thanks in advance.

share|improve this question

1 Answer

up vote 6 down vote accepted

  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.

share|improve this answer
2  
+0: did you remove the break; after SelectItemInComboBox() on purpose? :| – ANeves Nov 2 '11 at 16:35
2  
@ANeves: woops, nope. nice catch! – Steven Jeuris Nov 2 '11 at 20:04
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 Nov 2 '11 at 20:11
+1, now that that is fixed. – ANeves Nov 3 '11 at 10:04
2  
As a minor subtle (again subjective) side note. If SelectItemInComboBox is only called from one place, I wouldn't separate it in a function. – Steven Jeuris Nov 3 '11 at 10:53
show 2 more comments

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.