How NOT to Code - Example 8

Jan 22, 2010

So I mentioned that I would bounce around with languages a bit. Since many of us in the RIA development world do use Sql Server, I figure this would be nice to cover.

This is actually a fairly new one I learn at the local sql server user group. I actually would be surprised if anyone finds the problem in this one. This is really a dba level challenge, but it's good for programmers to know.


create procedure CustomerLookup
@CustomerID int = NULL
,@PersonID int = NULL

as
select * from Sales.Customer CU
inner join sales.salesorderheader sh on sh.customerid = cu.customerid
inner join sales.salesorderdetail sd on sd.salesorderID = sh.salesorderid
where (cu.customerid = @CustomerID or @CustomerID is NULL) and
    (cu.personID = @PersonID or @PersonID is NULL)

Comments

Dave Ferguson

Dave Ferguson wrote on 01/28/10 10:47 AM

Nice one here. This is most definitely a DBA challenge but I'll take a stab anyways.

First off.. the "select *" is a bad idea. Your queries should always only return the columns of data you need.

But, beyond that, it is a poorly constructed query. If I am reading this correctly the end result, if using default params of null, would return all rows where the joins succeed. I if the joins are a 1 to many you would end up with multiple rows for each customer. I am not a DBA so I don't know the precise reason why but I think it is because the where clauses cancel each other out.

--Dave
Katrina

Katrina wrote on 01/28/10 11:38 AM

The main thing change the where clause to:

where isnull(cu.customerid, 0) = @CustomerID and
isnull(cu.personID, 0) = @PersonID


Avoid select *

I would also consider breaking it up into separate results sets so you are not returning as much duplicate data

create procedure CustomerLookup
@CustomerID int = NULL
,@PersonID int = NULL

as


declare @count int

select [columns]
from Sales.Customer CU
where isnull(cu.customerid, 0) = @CustomerID
   and isnull(cu.personID, 0) = @PersonID
set @count = @@rowcount

if @count > 0
begin

   select [columns]
   from sales.salesorderheader sh
      inner join sales.salesorderdetail sd on sd.salesorderID = sh.salesorderid
   --no longer need the isnull because rowcount was > 0
   where sh.customerid = @CustomerID

end else begin
   select 'No records' as Result
end
spills

spills wrote on 01/28/10 12:26 PM

--- Comment Code Block
--- Added Use Nocount
--- BEGIN and END
--- no * unless good reason
--- added COALESCE to where and set value correctly as it does't make sense in orig. form
---
create procedure CustomerLookup

@CustomerID int = NULL,
@PersonID int = NULL

SET NOCOUNT ON
AS
BEGIN

select cu.col, sh.col
from Sales.Customer cu
inner join sales.salesorderheader sh on sh.customerid = cu.customerid
inner join sales.salesorderdetail sd on sd.salesorderID = sh.salesorderid
where (cu.customerid = COALESCE(@CustomerID,cu.cusomerID) and
(cu.personID = COALESCE(@PersonID,cu.personID )
END

Hope I didn't go to fast!
SET NOCOUNT OFF
Katrina

Katrina wrote on 01/28/10 12:52 PM

Also, likely the use of PersonId is superfluous. It seems the only use case to use it would be if there are customers who are not people (i.e. businesses).

Why are NULLS allowed for the Customer? Unless a person who is not a customer can have a sales order. This seems unlikely.

The name of the sp is poor. It implies a query on only the Customer table...better CustomerSalesOrderLookup

so I would change my first answer:

create procedure CustomerLookup
@CustomerID int --No default
,@PersonID int = NULL

declare @count int

select [columns]
from Sales.Customer CU
where cu.customerid = @CustomerID
and isnull(cu.personID, 0) = @PersonID
set @count = @@rowcount
Katrina

Katrina wrote on 01/28/10 3:44 PM

@spills

COALESCE ... Good catch!

I have used that before. Which means my WHERE clause is COMPLETELY WRONG.
John Mason

John Mason wrote on 02/09/10 8:43 AM

The issue here is the optional values allowed in this stored procedure. SQL Server caches the execution plan for a stored procedure after its first run. Because this has optional values, that caching is practically worthless and the result is poor performance. The solution is to break this up into a variety of stored procedures so you can eliminate the optional parameters.

Write your comment



(it will not be displayed)