SQL for Smarties | SQL Programming Style | Trees and Hierarchies in SQL | SQL Puzzles and Answers | Data and Databases


Monday, June 12, 2006

stop using dynamic sql

SQL Apprentice Question
The following stored procedure (SP) is using a dynamic sql to build the query.
How can this be written using a standard sql. i.e. NOT dynamically being
built.
Initially I thought I can have something like the following query but it
does not seem to be working for when the where caluse parameters are not
passed.
So I ended up using the dynamic sql as it returns the correct data.
Can you see how the SP can be altered please?
Thanks

------------------------this query does not return the correct data where
the where parameters are not passed.------------
select
*
from
tbl_Management
where


([Year] is null or [Year] = @Year)
AND
(YearPeriod is null or YearPeriod = @YearPeriod)
AND
(AreaCode is null or AreaCode = @AreaCode)


---------------------------------------------------------------


create procedure usp_PatientManagementAdminGet


@Year int = null,
@YearPeriod int = null,
@AreaCode varchar(3) = null


as


declare @sql varchar(1000)


set @sql = 'select'
set @sql = @sql + ' ID'
set @sql = @sql + ' ,AreaCode'
set @sql = @sql + ' ,Year'
set @sql = @sql + ' ,YearPeriod'
set @sql = @sql + ' ,A1=A2+A3'
set @sql = @sql + ' ,A2,A3'
set @sql = @sql + ' ,B1=B2+B3'
set @sql = @sql + ' ,X1=convert(int, ((B2+B3)*1.0/(A2+A3))*100)'
set @sql = @sql + ' from'
set @sql = @sql + ' tbl_Management'
set @sql = @sql + ' where'


if (@Year > 0)
begin
set @sql = @sql + ' [Year] = ' + convert(varchar(4), @Year)
set @sql = @sql + ' AND'
end
if (@YearPeriod > 0)
begin
set @sql = @sql + ' YearPeriod = ' + convert(varchar(2), @YearPeriod)
set @sql = @sql + ' AND'
end
if (@ProgrammeAreaCode is not null)
begin
set @sql = @sql + ' AreaCode = ''' + convert(varchar(3), @AreaCode) + ''''
set @sql = @sql + ' AND'
end


--trim off the last AND...
set @sql = left(@sql, len(@sql) - 3)


exec sp_sqlexec @sql


Celko Answers
Your real problem is that you still think in procedural terms and not
in declarative set-oriented programming.

Why did you think of dynamic SQL at all? (answer: you never un-learned
low level scripting or worked with a compiled language).


Why did you put redundant prefixes on data element names (answer: you
never un-learned BASIC, or other weakly typed languages or read
ISO-11179 naming rules).


Why did you avoid temporal data types? Answer: they do not exist in the
procedural language you mimic in SQL.


You do not understand data types and other constraints. Is there an
area code that varies in length? There will be in your data model!
Poor design invites garbage.


Why did you build the dynamic string in steps? Answer: procedural code
is done in steps; declarative code is only a few statements and they
execute all at once. Your bad code shows characteristic errors.


You have a magical, universal, vague and non-relational "id" in a
table because you do not know what a relational key is. It begs the
questions "id of what? How do I validate it? How do I verify it?"
Surely, you did not use IDENTITY as a key!! That would mimic a
sequential file and not anythign like an RDBMS.


Did you know that year is a reserved word in SQL? You would know that
if you had bothered to run a code check. But I will bet that you have
no such tool and that you do not keep a data dictionary either.


Why did you CAST() things so often in a strongly typed language?
Because you have never written in one before. Why do you use the
proprietary CONVERT to keep the code non-portable?


Why do you think that a1, b1, etc. are good data element names? Are
they standard in your trade? I doubt it. But you would know that if
you had a data dictionary.


Just cleaning up the code, we get some thing like this:


CREATE PROCEDURE GetPatientManagementAdmin
(@my_something_year INTEGER = NULL,
@my_year_period INTEGER = NULL,
@my_area_code CHAR(3) = NULL)
AS
SELECT patient_id, -- wild guess that this exists
area_code, something_year, year_period,
(a2+a3) AS a1, a2, a3,
(b2+b3) AS b1,
CAST(((b2+b3)*1.0/(a2+a3))*100)AS INTEGER)
AS x1 -- better name for all of this??
FROM Patient_Management
WHERE COALESCE (@my_ something_year, something_year) = something_year
AND COALESCE (@my_year_period, year_period) = year_period
AND = COALESCE (area_code, @my_area_code) = area_code;


But since you did not post DDL, we cannot solve the real problem; you
have a bad schema design. You are using integers for years!! You
should use (begin_date, end_date) pairs for temporal ranges. That would
probably mean an auxiliary calendar year That requires that you think
in terms of data solutions instead of procedural code.


Your year parameter probably ought to default to CURRENT_TIMESTAMP.

>> Also, if there is no practical useable natural key - like the Members table of a user group then using a IDENTITY for that is fine so long as you don't mind gaps. <<


Let's do the whole list of assumptions

1) do not mind gaps
2) do not need validation
3) do not need verification
4) will never scale up (Tony's no growth business model)
5) will never port your code- - no upgrades either! Daniel Wetzler
found out the hard way that IDENTITY changed behavior in SQL 2000 and
SQL 2005. If you perform the statement below you get only one dataset
which has the described properties.


SELECT DISTINCT IDENTITY (INTEGER) AS fake_id, title1, ..
FROM Foobar
WHERE title1 IS NOT NULL
AND ..


The IDENTITY function makes each row unique so DISTINCT doesn't
eliminate the duplicates in this case. Interestingly, this behavior
seems to have changed in SQL Server 2005. If I run this as a SELECT
INTO on 2005, the execution plan computes the IDENTITY value after
DISTINCT.


For 2000 the kludge is a bit hard to see. The following should insert
just one row into the target table.


CREATE TABLE Foobar (title1 VARCHAR(10), ..);


INSERT INTO Foobar VALUES ('1', ..);
INSERT INTO Foobar VALUES ('1', ..);


SELECT IDENTITY (INTEGER) AS fake_id, title1, ..
INTO Foobar2
FROM (SELECT DISTINCT title1, ..
FROM Foobar
WHERE ..);


Since we are dealling with a proprietary feature, this is subject to
change without noti ce again.


Of course, asking the members for their email address as their
identifier is never, ever done in the real world. Oh, wait -- it is!!
Do you suppose that there are other industry standard, well-known
identifiers in the world? Gasp!


Why, that would get around the problems mentioned above and be damn
near universal! But that is not as fast to code (e.g, requires a
CHECK() for a valid email address) as Tony's magical sequential number.
All you seem to care about is how fast you can code something, not
about the lifetime costs of maintaining the system.

No comments: