2
\$\begingroup\$

I have a form that allows the user to select a server. This will execute the exact same query just on a different server.

private void QueryButton_Click(object sender, EventArgs e)
{
    switch (this.FindByComboBox.SelectedIndex)
    {
        case 0:
            ByTargetVDN();
            break;
        // ...
    }
}
void ByTargetVDN()
{
    int value = 0;
    if (this.ValueTextBox.Text.Length > 0)
    {
        value = Convert.ToInt32(this.ValueTextBox.Text);
    }
    switch (this.ServerComboBox.SelectedIndex)
    {
        case 0:
            var gr01 = new GR01.TargetVDNDataTable();
            var adapter = new ICMQuery.Models.GR01TableAdapters.TargetVDNTableAdapter();
            if (value == 0)      
            {
                adapter.Fill(gr01);
            }
            else
            {
                adapter.FillByPeripheralNumber(gr01, value);
            }
            this.FindScriptsDataGrid.DataSource = gr01;
            break;
        case 1:
            var ds01 = new DS01.TargetVDNDataTable();
            var adapterNew = new ICMQuery.Models.DS01TableAdapters.TargetVDNTableAdapter();
            if (value == 0)
            {
                adapterNew.Fill(ds01);
            }
            else
            {
                adapterNew.FillByPeripheralNumber(ds01, value);
            }
            this.FindScriptsDataGrid.DataSource = ds01;
            break;
    }
}

There has got to be a way I can streamline this. I was hoping for something along the lines of 

switch (this.ServerComboBox.SelectedIndex)
  {
    case 0:
      var ds = new GR01.TargetVDNDataTable();
      var adapter = new ICMQuery.Models.GR01TableAdapters.TargetVDNTableAdapter();
      break;
    case 1:
      ds = new GR01.TargetVDNDataTable();
      adapter = new ICMQuery.Models.GR01TableAdapters.TargetVDNTableAdapter();
      break;
  }
if (value == 0)
  {
    adapter.Fill(ds);
  }
else
  {
    adapter.FillByPeripheralNumber(ds, value);
  }
this.FindScriptsDataGrid.DataSource = ds;

but of course this doesn't work.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ If you're running the same query, just on a different server, why are you filling separate datasets (In your working code). \$\endgroup\$ Commented Jul 1, 2014 at 22:14
  • \$\begingroup\$ @JohnP Thank you, I was using the wizard to generate these datasets and your comment made me think WTF am I doing this with the wizard, wrote the datasets in hard code instead and now I am happy with the code. Check this post: codereview.stackexchange.com/questions/55893/… \$\endgroup\$ Commented Jul 2, 2014 at 15:38

2 Answers 2

Reset to default
3
\$\begingroup\$ 
if (this.ValueTextBox.Text.Length > 0)
{
    value = Convert.ToInt32(this.ValueTextBox.Text);
}

This will throw a nullpointerexception if Text is null, use TryParse instead

int value;
bool result = Int32.TryParse(ValueTextBox.Text, out value);
if(result){
 // use value
}

I don't recommend you using SelectedIndex, because it breaks your code if the ordering get changed in the future, I would use SelectedItem instead.

Notice that you are repeating code, which is not a brilliant idea.

TheTypeForThis gr01 = null;
TheTypeForThis adapter = null;
switch (this.ServerComboBox.SelectedIndex)
{
    case 0:
        gr01 = new GR01.TargetVDNDataTable();
        adapter = new ICMQuery.Models.GR01TableAdapters.TargetVDNTableAdapter();
        break;
    case 1:
        var ds01 = new DS01.TargetVDNDataTable();
        var adapterNew = new ICMQuery.Models.DS01TableAdapters.TargetVDNTableAdapter();
       break;
}

 if (value == 0)      
 {
    adapter.Fill(gr01);
 }
 else
 {
    adapter.FillByPeripheralNumber(gr01, value);
 }
 this.FindScriptsDataGrid.DataSource = gr01;
\$\endgroup\$
2
  • \$\begingroup\$ I am aware of the code repetition hence the post :D as far as the first part, it doesn't throw exceptions because if the text was null the length would be less than 0. I'll check out the SelectItem that looks promising. Your last bit of code completely missed the point of the post. \$\endgroup\$ Commented Jul 1, 2014 at 21:56
  • \$\begingroup\$ @aaronmallen there you go :D \$\endgroup\$ Commented Jul 1, 2014 at 21:57
-1
\$\begingroup\$
  • I assume it's your style to refer to all class using "this.". That's not the usual way to do things but it is acceptable.
  • It also seems to be your style to capitalize the names of the UI elements. Again, not the usual style.
  • You really need to separate the UI and the business logic. The way things stand now, it would be impossible to drive the program via an API or a batch process. Especially notable since that's usually the best way to test it.
  • Especially since you haven't included the definitions of classes GR01 and DS01 are, it's impossible to tell from this code snippet what gr01 and ds01 are.
  • The biggest problem seems to be that you're declaring adapter and adapternew inside the switch. Move those declarations outside, and you should only need one variable.
\$\endgroup\$
1
  • \$\begingroup\$ anyone care to explain why I'm getting multiple -1's here? \$\endgroup\$ Commented Jul 2, 2014 at 20:02

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.