2
\$\begingroup\$

I have a c# application(mvc) that allows a user to upload an excel sheet containing an account number key and amount value to be passed as parameters to a proc that then validates those records, the proc then return an error or warning if any are found. My issue is that the BI team implemeted a proc which I cannot modify to make it accept a table type as explained in this post Table Value

The issue is that I cannot modify the proc to accept an table type, so I need to optimize the stored procedure call in c# code. The performance is acceptable if there are about 30 to 50 lines the proc needs to validate, but once there are more record e.g a 100 or more the application is slow, is there a way in c# that I can improve the performance of this code, please see my implementation below.

using (SqlConnection conn = new SqlConnection(sqlConnection))
    {

        try
        {

            foreach (var claim in supplierClaimsData)
            {
                SqlCommand cmd = new SqlCommand();
                cmd.CommandTimeout = 60;
                SqlDataReader reader;
                cmd.Connection = conn;
                cmd.CommandText = "CRM.Supplier_Claim_Upload";
                cmd.CommandType = CommandType.StoredProcedure;

                cmd.Parameters.Add("@Invoice", SqlDbType.NVarChar).Value = claim.Line_Number;
                cmd.Parameters.Add("@Amount", SqlDbType.Decimal).Value = claim.Total_Claim;

                conn.Open();
                reader = cmd.ExecuteReader();

                while (reader.Read())
                {
                    claim.Error_1 = reader.GetString(reader.GetOrdinal("Error1"));

                    string lineNumberDoesNotExist = "Error: Invoice line number does not exist";
                    if (claim.Error_1.StartsWith(lineNumberDoesNotExist))
                    {
                        continue;
                    }

                    claim.Warning = reader.GetString(reader.GetOrdinal("Warning"));
                    claim.Error_2 = reader.GetString(reader.GetOrdinal("Error2"));

                }


                conn.Close();


            }


            foreach (CleanSupplierClaim saveToDBClaim in supplierClaimsData)
            {
                db.CleanSupplierClaims.Attach(saveToDBClaim);

                var entry = db.Entry(saveToDBClaim);
                entry.Property(aa => aa.Line_Number).IsModified = true;
                entry.Property(aa => aa.Total_Claim).IsModified = true;
                entry.Property(aa => aa.Currency).IsModified = true;
                entry.Property(aa => aa.ClaimReference).IsModified = true;
                entry.Property(aa => aa.Action).IsModified = true;
                entry.Property(aa => aa.Domain_Username).IsModified = true;
                entry.Property(aa => aa.Error_1).IsModified = true;
                entry.Property(aa => aa.Error_2).IsModified = true;
                entry.Property(aa => aa.Warning).IsModified = true;
                entry.Property(aa => aa.ImportFlag).IsModified = true;
                entry.Property(aa => aa.ReadyForImport).IsModified = true;
                db.Entry(saveToDBClaim).State = System.Data.Entity.EntityState.Modified;
                db.SaveChanges();


            }


        }

I would really appreciate your input in this regard.

Here is the procedure I got from the QA sql, it basically validates claims with Invoice number and Amount as parameters.

    ALTER PROCEDURE [CRM].[Supplier_Claim_Upload] 
(
@Invoice        varchar(20), 
@Amount         decimal(18,3) 
)
as
set nocount on

declare         @ST_Key varchar(20)     = @Invoice
declare         @Claim decimal(18,3)    = @Amount

select * into #Temp
from (
        select st.ST_Key, 
                case when sm.Supplier_Claim_Original > 0 then 'Warning: You Are going to Overwrite a Claim Value: ' + convert(varchar(50), round(sm.Supplier_Claim_Original, 0)) else '' end as 'Warning',
                case when st.Orig_Inv is not null then 'Error: You are attempting to claim against a returned invoice: ' + st.Orig_Inv else '' end as 'Error1',
                case when  @Claim > sm.System_Cost then 'Error: Claim value is higher than system cost: ' + convert(varchar(50), sm.System_Cost) else '' end as 'Error2'
        from Embrace.fact.Sales st
        left join Embrace.fact.Sales_Margin sm on sm.ST_Key = st.ST_Key and sm.Tarsus_Country = st.Tarsus_Country
        where st.ST_Key = @ST_Key
) x

if not exists (select top 1 * from #Temp)
begin
    select @ST_Key as 'ST_Key', 'Error: Invoice line number does not exist' as 'Error1'
end

else

begin
    select * from #Temp 
end

drop table #Temp

--Action    
--Line No   
--Total Claim   
--Currency  
--Claim Reference


set nocount off
\$\endgroup\$
  • 1
    \$\begingroup\$ I know you cannot change the stored procedure, but maybe you could show its code anyway? Perhaps some is able to tell whether it's even possible to optimize calls to it. \$\endgroup\$ – t3chb0t Sep 6 '16 at 16:48
  • \$\begingroup\$ @t3chb0t I have no access to the proc itself. I will ask the BI team and post it here. I was just wondering if there was a way to optimize my current code in c# \$\endgroup\$ – Papi Sep 12 '16 at 14:24
  • \$\begingroup\$ You are not doing anything extraordinary there. You just call the procedure with the requried parameters and you read the results. I don't see much room (if any) for optimization. If something is slow then most probably it will be the procedure itself. Maybe it contains serveral weird joins without real foreign keys or something. \$\endgroup\$ – t3chb0t Sep 12 '16 at 14:29
  • \$\begingroup\$ @t3chb0t I have added the procedure code that I got from the QA SQL box, please have a look and advise, remember I did not write this proc and if there is anything that can be improbved I will appreciate it and I will communicate with the BI team to improve. \$\endgroup\$ – Papi Sep 12 '16 at 14:48
2
\$\begingroup\$

I think this is a very strange way to validate anything and it's extremely unmaintainable.

Why don't you (the BI team) just return an error code (and maybe additional information in other columns). The error messages are no help at all because you need to use the same strings in your application to check which error message it is. If they correct a typo or change a letter or two or add another word then your app doesn't work anymore.

Even more strange the error code are actually your columns. So you have three error codes (columns): Warning, Error1, Error2 where the Error1 can be either there is no invoice or you are attempting to claim....

If you used error codes you could define an enum or constants and you instantly would know what's going on with the invoce.


As to the optimization... You cannot optimize it if you have to ask the database for each invoice. Tell the BI team that they should let you pass a bunch of invoice numbers (maybe a 100 or more at a time) to the stored procedure and give you back the error codes for them. This way you would already have saved a 100 database roundtrips.

There are several ways to accomplish it, see How to pass an array into a SQL Server stored procedure;


You shouldn't do everything on each loop. Open the connection once before the loop, create the command once before the loop starts too. You can even add the parameters. Inside the loop you should only set their values:

using (var cmd = new SqlCommand())
{
    cmd.CommandTimeout = 60;
    cmd.Connection = conn;
    cmd.CommandText = "CRM.Supplier_Claim_Upload";
    cmd.CommandType = CommandType.StoredProcedure;

    var invoiceParam = cmd.Parameters.Add("@Invoice", SqlDbType.NVarChar);
    var amountParam = cmd.Parameters.Add("@Amount", SqlDbType.Decimal);

    conn.Open();

    foreach (var claim in supplierClaimsData)
    {

        invoiceParam.Value = claim.Line_Number;
        amountParam.Value = claim.Total_Claim;

        using (var reader = cmd.ExecuteReader())
        {
            while (reader.Read())
            {
                ...
            }
        }
    }
}
\$\endgroup\$
1
\$\begingroup\$

Well, I don't see any

using

around

new SqlCommand()

I would try to use a basic using pattern around the new SqlCommand and would test it. SqlCommand inherits from DbCommand and implements IDisposable, so I will at least try to call a Cmd.dispose() in such a case.

Hope this helps

\$\endgroup\$

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.