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

Validate and Add Service Provider/Location in C#

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

Problem

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

Solution

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

Code Snippets

bll.LocationID = ddlLocations.SelectedValue;
bll.IsApproved = "y";
bll.ServiceProviderLocationAdd();
gvServiceProviders.DataSource = bll.GetServiceProviderLocations();
gvServiceProviders.DataBind();
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();
        }
    }       
}
bll.LocationID = ddlLocations.SelectedValue;
bll.IsApproved = "y";
bll.ServiceProviderLocationAdd();

Context

StackExchange Code Review Q#79665, answer score: 8

Revisions (0)

No revisions yet.