Validate and Add Service Provider/Location in C#

Question

I have a button that will validate the textbox for the service provider and the dropdown list for the locations. I have 3 tables: Locations, Service_Providers, and Service_Providers_Locations with a many-to-many relationship. But the tables that are affected are mainly Service_Providers and Service_Providers_Locations. However, upon running, the button inserts the record even if there are duplicates. What we want is to not allow duplicate record in the transaction table Service_Providers_Locations. There can be the same name of service provider, but not same location. Our code allows duplicate record insertion. We are using a three-tier approach.

protected void btnAddServiceProvider_Click(object sender, EventArgs e)
 {
 bll.Name = txtServiceProvider.Text;
 DataTable temp = bll.ServiceProviderCheckExisting();
 if (temp.Rows.Count == 0)
 {
 bll.IsDeleted = "n";
 bll.ServiceProviderAdd();
 temp = bll.ServiceProviderCheckExisting();
 bll.ServiceProviderID = temp.Rows[0][0].ToString();
 bll.LocationID = ddlLocations.SelectedValue;
 bll.IsApproved = "y";
 bll.ServiceProviderLocationAdd();
 gvServiceProviders.DataSource = bll.GetServiceProviderLocations();
 gvServiceProviders.DataBind();
 }
 else
 {
 bll.ServiceProviderID = temp.Rows[0][0].ToString();
 temp = bll.GetServiceProviderLocationUsingServiceProviderID();
 if (temp.Rows.Count == 0)
 {
 bll.LocationID = ddlLocations.SelectedValue;
 bll.IsApproved = "y";
 bll.ServiceProviderLocationAdd();
 gvServiceProviders.DataSource = bll.GetServiceProviderLocations();
 gvServiceProviders.DataBind();
 }
 else
 {
 for (int x = 0; x < temp.Rows.Count; x++)
 {
 if (ddlLocations.SelectedValue == temp.Rows[x][1].ToString())
 {
 lblServiceProviderError.Text = "This service provider already exists for that location.";
 }
 else
 {
 bll.LocationID = ddlLocations.SelectedValue;
 bll.IsApproved = "y";
 bll.ServiceProviderLocationAdd();
 gvServiceProviders.DataSource = bll.GetServiceProviderLocations();
 gvServiceProviders.DataBind();
 }
 }
 }
 }
 }

• Heslacher: Does this code insert duplicates ?
• Jeano Ermitaño: @Heslacher yes it does. I believe that the for loop is responsible for this. But I have yet to figure out how and why.
• Heslacher: Would you mind reformatting the text a little bit, so that it is easier to read ?

Answer 1

There are five lines that are repeated three times:

bll.LocationID = ddlLocations.SelectedValue;
bll.IsApproved = "y";
bll.ServiceProviderLocationAdd();
gvServiceProviders.DataSource = bll.GetServiceProviderLocations();
gvServiceProviders.DataBind();

Those should be moved to a separate method to start with. I also don't like that it does two things: update bll and then use bll to update gvServiceProviders; this looks like a mix of UI and business logic.

---

temp is a really bad name.

---

At this point we're basically here, if you apply some techniques to reduce the indentation:

protected void btnAddServiceProvider_Click(object sender, EventArgs e)
{
 bll.Name = txtServiceProvider.Text;
 DataTable temp = bll.ServiceProviderCheckExisting();
 if (temp.Rows.Count == 0)
 {
 bll.IsDeleted = "n";
 bll.ServiceProviderAdd();
 temp = bll.ServiceProviderCheckExisting();
 bll.ServiceProviderID = temp.Rows[0][0].ToString();
 Update();
 return;
 }
 bll.ServiceProviderID = temp.Rows[0][0].ToString();
 temp = bll.GetServiceProviderLocationUsingServiceProviderID();
 if (temp.Rows.Count == 0)
 {
 Update();
 return;
 }
 for (int x = 0; x < temp.Rows.Count; x++)
 {
 if (ddlLocations.SelectedValue == temp.Rows[x][1].ToString())
 {
 lblServiceProviderError.Text = "This service provider already exists for that location.";
 }
 else
 {
 Update();
 }
 } 
}

(Update() is the method that contains the five lines I mentioned at the beginning. Not a great method name either.)

Which I find to be an odd flow, to say the least. Which is partly due to not knowing what bll actually is.

To me it feels like some of the code should be re-thought. For instance this:

bll.LocationID = ddlLocations.SelectedValue;
bll.IsApproved = "y";
bll.ServiceProviderLocationAdd();

Why isn't this one single method on bll that takes the value of ddlLocations.SelectedValue as a parameter?

I'm also not fond of ServiceProviderCheckExisting() returning a DataTable, I'd expect it to be more like GetExisting().

Answer 2

I have placed the insertion outside of the for loop and created a boolean variable that will change when the program in the for loop runs in the first if statement. It now works as I wanted it to be.

bool isExisting = false;
for (int x = 0; x < temp.Rows.Count; x++)
{
 if (ddlLocations.SelectedValue == temp.Rows[x][1].ToString())
 {
 lblServiceProviderError.Text = "Service provider already exists.";
 isExisting = true;
 }
 else
 {
 continue;
 }
}
if (isExisting == false)
{
 bll.LocationID = ddlLocations.SelectedValue;
 bll.IsApproved = "y";
 bll.ServiceProviderLocationAdd();
 gvServiceProviders.DataSource = bll.GetServiceProviderLocations();
 gvServiceProviders.DataBind();
}

• BCdotWEB: The continue; in the else is superfluous, and thus the whole else block. Nobody writes if (isExisting == false), instead we write if (!isExisting). Even better would be to avoid this negative check and invert the logic, i.e. bool addProviderLocation = true; etc. so you'd get if (addProviderLocation).