6
\$\begingroup\$

I was wondering if you would be willing to give me some suggestions on shortening this code. I feel as if the amount of if statements I have is a bit much.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Speech.AudioFormat;
using System.Speech.Synthesis;
using System.Text;
using System.Threading.Tasks;
using System.Windows;
using System.Windows.Controls;
using System.Windows.Data;
using System.Windows.Documents;
using System.Windows.Input;
using System.Windows.Media;
using System.Windows.Media.Imaging;
using System.Windows.Navigation;
using System.Windows.Shapes;

namespace SpeechTutorial
{
    /// <summary>
    /// Interaction logic for MainWindow.xaml
    /// </summary>
    public partial class MainWindow : Window
    {
        SpeechSynthesizer synth = new SpeechSynthesizer();

        public MainWindow()
        {
            InitializeComponent();
            comboItems();
        }

        private void comboItems()
        {
                foreach (InstalledVoice voice in synth.GetInstalledVoices())
                {
                    // loop through each voice installed on machine
                    VoiceInfo info = voice.VoiceInfo;
                    // adds each installed voice's Name and location to the combobox's list
                    comboBox.Items.Add(info.Description);
                }
            }

        private void button_Click(object sender, RoutedEventArgs e)
        {
            // set the synthesizer to speak
            synth.Speak(textBox.Text);
        }

        private void comboBox_SelectionChanged(object sender, SelectionChangedEventArgs e)
        {
            // here we are setting the voices for each selected item in the combobox

            try
            {
                if (comboBox.SelectedIndex == 0)
                {
                    synth.SelectVoice("Microsoft Hazel Desktop");
                }

                if (comboBox.SelectedIndex == 1)
                {
                    synth.SelectVoice("Microsoft Heera Desktop");
                }

                if (comboBox.SelectedIndex == 2)
                {
                    synth.SelectVoice("Microsoft David Desktop");
                }

                if (comboBox.SelectedIndex == 3)
                {
                    synth.SelectVoice("Microsoft Zira Desktop");
                }

                if (comboBox.SelectedIndex == 4)
                {
                    synth.SelectVoice("Microsoft Haruka Desktop");
                }

                if (comboBox.SelectedIndex == 5)
                {
                    synth.SelectVoice("Microsoft Heami Desktop");
                }

                if (comboBox.SelectedIndex == 6)
                {
                    synth.SelectVoice("Microsoft Huihui Desktop");
                }

                if (comboBox.SelectedIndex == 7)
                {
                    synth.SelectVoice("Microsoft Tracy Desktop");
                }

                if (comboBox.SelectedIndex == 8)
                {
                    synth.SelectVoice("Microsoft Hanhan Desktop");
                }
            }
            catch (Exception ex)
            {
                MessageBox.Show(ex.Message);
            }

        }
    }
}
\$\endgroup\$
10
\$\begingroup\$

One way to remove that ugly block of ifs is to use a string[]. You'd define it like this:

string[] voices = {
    "Microsoft Hazel Desktop",
    "Microsoft Heera Desktop",
    // etc.
};

Then, in place of your ifs, write this:

synth.SelectVoice(voices[comboBox.SelectedIndex]);

Much cleaner.

Also, C# convention states that method names are PascalCase, not camelCase. So, for example, comboItems should be ComboItems.

Aside from that, it looks good. To me, at least.

\$\endgroup\$
11
\$\begingroup\$

Take a look at arrays. It will allow you to quickly add and remove future options without needing to change the logic of the code. It will also allow you to possibly add more complex logic in a single place, instead of having to add code to every if/switch statements.

string[] options = { "Microsoft Hazel Desktop",
                     "Microsoft Heera Desktop",
                     "Microsoft David Desktop",
                     "Microsoft Zira Desktop",
                     "Microsoft Haruka Desktop",
                     "Microsoft Heami Desktop",
                     "Microsoft Huihui Desktop",
                     "Microsoft Tracy Desktop",
                     "Microsoft Hanhan Desktop"};

try
{
    synth.SelectVoice(options[comboBox.SelectedIndex]);
}
catch (Exception ex)
{
    MessageBox.Show(ex.Message);
}
\$\endgroup\$
  • \$\begingroup\$ I learn something new every day... \$\endgroup\$ – KidCode Jun 7 '15 at 20:45
  • 2
    \$\begingroup\$ why not just use a string array? if each key is correspends to the index. \$\endgroup\$ – BjarkeCK Jun 7 '15 at 21:00
  • \$\begingroup\$ this is an approach I've never thought of. Nor have I tried using a Dictionary in C# yet. This is an awesome idea and learning experience all at once, thank you! \$\endgroup\$ – Jesse Glover Jun 7 '15 at 21:20
  • \$\begingroup\$ As per @BjarkeCK's comment, this would be far better handled with an array, rather than a dictionary. Also, -1 for using an out parameter. \$\endgroup\$ – David Arno Jun 7 '15 at 22:00
  • \$\begingroup\$ @David Arno why would an array better suit this example than Dictionary<int, string>? \$\endgroup\$ – Jesse Glover Jun 7 '15 at 23:01
3
\$\begingroup\$

Naming

Based on the naming guidelines Methods should be named using verbs or verb phrases and should be named using PascalCase casing.
So the method comboItems() should be named like e.g FillComboBoxItems().

Comments

Here I will direct you to this very good answer about comments: Guessing a number, but comments concerning

Comments are supposed to make plain what the code does not tell us already.

So comments which are telling what we already see from the code is not adding any value to your code but a lot of noise.

        // set the synthesizer to speak
        synth.Speak(textBox.Text);  

Multiple if's

Instead of multiple if statements which are checking a constant value you should better use a switch..case statement like

switch(comboBox.SelectedIndex)
{
    case 0:
        synth.SelectVoice("Microsoft Hazel Desktop");
        break;
    case 1:
        synth.SelectVoice("Microsoft Heera Desktop");
        break;
    ......
}  

Try..catch

Using try..catch in this szenario is just useless. What part of the code would throw here ? Only a case where you have accidently a typo in the filled in name or if a voice you want to select isn't installed. This is an exception which is better never thrown and should be eleminated before this code is called by checking if the desired voice is an installed one.

The best exception is an avoided exception.


The showed solution of @PaysTaxes is a way to go, but could be improved by using databinding.

First we create a MainWindowDataContext class which holds our collections we want to use for the databinding.

public class MainWindowDataContext
{
    public System.Collections.ObjectModel.ReadOnlyCollection<InstalledVoice> InstalledVoices { get; private set; }
    public MainWindowDataContext(SpeechSynthesizer synthesizer)
    {
        InstalledVoices = synthesizer.GetInstalledVoices();
    }
}  

We pass a SpeechSynthesizer object inside the constructor so we can call the GetInstalledVoices() method and store the result in the InstalledVoices property.

Next we add the xaml for the binding like

<Window x:Class="WpfApplication1.MainWindow"
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
        Title="MainWindow" Height="350" Width="525">
    <Grid>
        <ComboBox Height="38" HorizontalAlignment="Left" Margin="86,51,0,0" Name="comboBox" VerticalAlignment="Top" Width="253" 
        ItemsSource="{Binding Path=InstalledVoices}"     
        DisplayMemberPath="VoiceInfo.Description"
        SelectedValuePath="VoiceInfo.Name"   
        SelectionChanged="comboBox_SelectionChanged" />
    </Grid>
</Window>

And use this like

public partial class MainWindow : Window
{
    SpeechSynthesizer synthesizer = new SpeechSynthesizer();
    public MainWindow()
    {
        InitializeComponent();
        DataContext = new MainWindowDataContext(synthesizer);
    }

    private void comboBox_SelectionChanged(object sender, SelectionChangedEventArgs e)
    {
        ComboBox box = sender as ComboBox;
        if (box == null)
        {
            return;
        }

        synthesizer.SelectVoice(box.SelectedValue.ToString());
    }
}
\$\endgroup\$
1
\$\begingroup\$

I would like to point to one thing I did not see anyone suggesting. While forgetting better solutions like switch cases or string arrays, in your if-series, you are checking selected index of combobox 9 times. And that's painful. In all the cases it will do 9 comparisons. If you go for else...if it will do only one comparison in best case and 9 comparisons in worst case. If "saving" matters to you, then this is for you:

                if (comboBox.SelectedIndex == 0)
                {
                    synth.SelectVoice("Microsoft Hazel Desktop");
                }
                else if (comboBox.SelectedIndex == 1)
                {
                    synth.SelectVoice("Microsoft Heera Desktop");
                }
                else if (comboBox.SelectedIndex == 2)
                {
                    synth.SelectVoice("Microsoft David Desktop");
                }
                else if (comboBox.SelectedIndex == 3)
                {
                    synth.SelectVoice("Microsoft Zira Desktop");
                }
                else if (comboBox.SelectedIndex == 4)
                {
                    synth.SelectVoice("Microsoft Haruka Desktop");
                }
                else if (comboBox.SelectedIndex == 5)
                {
                    synth.SelectVoice("Microsoft Heami Desktop");
                }
                else if (comboBox.SelectedIndex == 6)
                {
                    synth.SelectVoice("Microsoft Huihui Desktop");
                }
                else if (comboBox.SelectedIndex == 7)
                {
                    synth.SelectVoice("Microsoft Tracy Desktop");
                }
                else if (comboBox.SelectedIndex == 8)
                {
                    synth.SelectVoice("Microsoft Hanhan Desktop");
                }
\$\endgroup\$
  • \$\begingroup\$ I believe using an array is superior to this alternative. \$\endgroup\$ – Simon Forsberg Sep 2 '15 at 9:40
  • 1
    \$\begingroup\$ @SimonAndréForsberg, I just tried to illustrate benefit of else if in the code. I agree using array is better. \$\endgroup\$ – sangam Sep 2 '15 at 9:58
0
\$\begingroup\$

A switch statement may be better, but you can reduce the code size and use a static data structure instead, like a map. In C#, you can do this with a Dictionary.

\$\endgroup\$
  • \$\begingroup\$ Note that while the other answer is correct, the collection initialization runs every time the handler is called, so you can declare it as a private static readonly and initialize it in a static constructor to avoid this slight performance loss (afaik this is not optimized away automatically), and to clarify that you are just searching the collection and not modifying it. \$\endgroup\$ – gorhawk Jun 7 '15 at 21:01
-1
\$\begingroup\$

Use a switch statement and pass in the comboBox.SelectedIndex as the parameter.

switch(comboBox.SelectedIndex)
{
    case 1:
        //do stuff
        break;
    case 2:
        //do stuff
        break;
    default:
        //do more stuff
        break;
}

case [value here]: means:

When the value of 'comboBox.SelectedIndex' is 1, execute the statements below and then return from the switch.

default: means:

If the value is different to any case, then execute this instead. (This is optional!)

On another note, your only using if statements, meaning if the index is 3, it executes the code for 3 and then still runs through the rest of the if statements, all of which will return false. -A negative on performance.

In cases like this where you are testing a value against multiple conditions where only one can be true, it is always better to use an if-else or switch construct.

\$\endgroup\$
  • 1
    \$\begingroup\$ Switch is never the right answer to any question \$\endgroup\$ – David Arno Jun 7 '15 at 21:56
  • \$\begingroup\$ @David Arno - Could you explain? \$\endgroup\$ – KidCode Jun 8 '15 at 6:08
  • 2
    \$\begingroup\$ @KidCode, switch statements, as a replacement to if-else's, reduce the risk of errors of mistyping the expression, but they create noise (eg the need to spread break throughout the code), they do not reduce cyclomatic complexity (requiring more testing) and this encourage long, deeply nested code as much as a string of if-else's do. Using a value or lambda lookup mechanism via a Dictionary or - in this case - an array greatly reduces cyclomatic complexity, discourages long-winded code, and results in easier-to-read and easier-to-test code. \$\endgroup\$ – David Arno Jun 8 '15 at 8:48
-1
\$\begingroup\$

Maybe, better use switch than many if.

\$\endgroup\$
  • 2
    \$\begingroup\$ Switch is never the right answer to any question. \$\endgroup\$ – David Arno Jun 7 '15 at 21:56
  • \$\begingroup\$ @DavidArno, could you expand little with your answer please? Would be helpful to us all. \$\endgroup\$ – sangam Sep 2 '15 at 9:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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