Please start any new threads on our new site at https://forums.sqlteam.com. We've got lots of great SQL Server experts to answer whatever question you can come up with.

 All Forums
 General SQL Server Forums
 New to SQL Server Programming
 Please critique my work

Author  Topic 

LaurieCox

158 Posts

Posted - 2007-02-23 : 12:00:15
Hi,

I would really appreciate any constructive criticism of my sql script (see end of message).

I have been working for a week to pull data from a database that has evolved over the years. It might have been designed to begin with, but it has had a lot of changes made on the fly, which makes pulling some data from it very hard.

One example is that they wanted Primary Service for a given client but this data was not stored in a base client table, but was instead on a table with multiple rows per client. To pick the row that referenced the correct Primary Service I had to:
1. Join the Client table with the view on client number and primary program number
2. From the records returned look at another column (proc_cde) in the view and pick the one that had a higher priority than the others. The precedence order of the proc_cdes was determined by a list my boss gave me. I used this forum to help me figure this part out.

Anyway … I finally got it done and it works … but just because it works doesn't mean it is well written. What I would like is if some of you could take a look at the sql and critique it for good and/or bad practices.

The design of the database is a given (and not my doing) so I am not looking for a critique of it.

An Example of what I am asking for would be:
I ended up using temp tables to collect some of the more complicated data so that I could write a very straightforward final query. Was this a good or bad thing and if bad why?

I really want to get good at sql and avoid developing any bad habits, so please critique my sql.

Also please ask me any questions that could help you evaluate the code.

Thanks,

Laurie

Note: the comments at the top were all the specs I got for creating this report (they came in an email).

/*detail- primary clincian/provider priemp_num joined to facemp Y
primary service (act,cm,opt), pripgm_num primary program,
select proc_cde from v_autsvc join svc
on proc_cde where pgm_num=pripgm_num
H0039 then T1017 t1016 then H0036
location of service, pgm_num of 90862 auth
date of auth v_autsvc.aut_dte
cons name/number: CLTCLT
auth date range v_autsvc.autbeg_dte v_autsvc.autend_dte
number of claims in the last fiscal year v_clmsvc, count(*)
primary psycchiatrist. cltdmo.pspemp_num join phy on phy_num


select clt_num, count(*) as num_claims from v_clmsvc
where beg_dte between '10/1/2005' and '9/30/2006 23:59'
group by clt_num

tables: v_autsvc, pgm, cltdmo.population='mi adult'
proc_cde in ('90862','90801')
autend_Dte>'10/1/06'

I need a report showing all Adult MI consuemrs receiving psych services by agency- like the
aggregate and detail of who gets them at where-bridgeways, sr srv... including our clinic.

i would need in the detail- primary clincian/provider, primary service (act,cm,opt), location of service,
date of auth, cons name/number, auth date range/# of units and number of claims in the last fiscal year and
primary psycchiatrist. if xxx asks for something similar, let me know as we might be duplicating.

i know that xxx is going to be asking for a report that is for just our clinic that has more to do with
insurance type. if it would be easier, you could combine the 2 reports as long as we have the capacity
to sort and print by provider. thanks*/

/**********************************************************************************
Primary Service
***********************************************************************************/
-- select primary proc_cde for primary service Candidates
drop table #ConsumerProcCodes
select distinct a.clt_num,
a.proc_cde,
a.aut_dte,
a.autbeg_dte,
a.autend_dte
into #ConsumerProcCodes
from cltdmo d join v_autsvc a on a.clt_num = d.clt_num
and d.pripgm_num = a.pgm_num
and a.autsts = 1
and getdate() between a.autbeg_dte and a.autend_dte


-- create table of ProcCde Precedence
drop table #ProcCde_Precedence
create table #ProcCde_Precedence
(proc_cde varchar(10),
Precedence int)

insert #ProcCde_Precedence
(proc_cde,Precedence)
select 'H0039', 10 union all
select 'T1017', 20 union all
select 'T1016', 30 union all
select 'H0036', 40 union all
select 'T2011', 50 union all
select '90806', 60 union all
select '90862', 70 union all
select 'T1002', 80 union all
select 'H2031', 90 union all
select 'H2023', 100 union all
select 'H2015', 110 union all
select 'T1005', 120

-- assign precedence to proc_cde
drop table #ConsumerProcCodesWPrec
select distinct c.clt_num,
c.aut_dte,
c.autbeg_dte,
c.autend_dte,
case when p.Precedence is null then 1000
else p.Precedence
end as Precedence,
left(c.proc_cde,5) as proc_cde
into #ConsumerProcCodesWPrec
from #ConsumerProcCodes c left join #ProcCde_Precedence p on p.proc_cde = left(c.proc_cde,5)

-- select primary proc_cde for each consumer and get associated PrimaryService
drop table #PrimaryService
select distinct p1.clt_num,
p1.proc_cde,
p1.aut_dte,
p1.autbeg_dte,
p1.autend_dte,
s.acttyp_des as PrimaryService
into #PrimaryService
from #ConsumerProcCodesWPrec p1 join svc s on p1.proc_cde = s.proc_cde
where Precedence = (select min(Precedence)
from #ConsumerProcCodesWPrec p2
where p1.clt_num = p2.clt_num)

-- Find Consumers with more than one primary Proc_cde
/*
select clt_num, proc_cde, aut_dte, autbeg_dte, autend_dte
from #PrimaryService
where clt_num in (select clt_num
from #PrimaryService
group by clt_num
having sum(1) > 1)
*/
/**********************************************************************************
Primary Location
***********************************************************************************/
-- select pgm_num for Primary location Candidates
drop table #ConsumerLOS
select distinct a.clt_num,
a.pgm_num,
p.pgm_nme
into #ConsumerLOS
from v_autsvc a join pgm p on a.pgm_num = p.pgm_num
where a.autsts = 1
and getdate() between a.autbeg_dte and a.autend_dte
and a.proc_cde = '90862'

-- create table of pgm_num for Location of Services Precedence
drop table #LOS_Precedence
create table #LOS_Precedence
(pgm_num int,
Precedence int)

insert #LOS_Precedence
(pgm_num,Precedence)
select 5600, 10 union all
select 6200, 20 union all
select 6000, 30 union all
select 1611, 40 union all
select 1612, 50 union all
select 1601, 60

-- assign precedence to pgm_num
drop table #ConsumerLOSwPrecedence
select distinct c.clt_num,
case when p.Precedence is null then 1000
else p.Precedence
end as Precedence,
c.pgm_num
into #ConsumerLOSwPrecedence
from #ConsumerLOS c left join #LOS_Precedence p on p.pgm_num = c.pgm_num


-- select pgm_num for primary location of Services for each consumer
drop table #PrimaryLOS
select distinct p1.clt_num,
case n.NumRows
when 1 then cast(p1.pgm_num as varchar(10))
else cast(p1.pgm_num as varchar(10)) + '*'
end as LocationOfService
into #PrimaryLOS
from #ConsumerLOSwPrecedence p1 join (select clt_num,count(*) as NumRows
from #ConsumerLOSwPrecedence
group by clt_num) n on p1.clt_num = n.clt_num
where Precedence = (select min(Precedence)
from #ConsumerLOSwPrecedence p2
where p1.clt_num = p2.clt_num)



-- Find Consumers with more than one primary pgm_cde for Location of Services.
/*select distinct clt_num from #PrimaryLOS
select * from #PrimaryLOS
where clt_num in (select clt_num
from #PrimaryLOS
group by clt_num
having sum(1) > 1)*/

/**********************************************************************************
Put it all together
***********************************************************************************/
drop table #FinalResults
select distinct c.clt_num,
c.fst_nme + ' ' + c.lst_nme as ConsumerName,
f.fst_nme + ' ' + f.lst_nme as PrimaryClinician,
p.fst_nme + ' ' + p.lst_nme as PrimaryPsychiatrist,
ps.PrimaryService,
COALESCE(n.num_claims,0) as num_claims,
los.LocationOfService,
ps.aut_dte,
ps.autbeg_dte,
ps.autend_dte
into #FinalResults
from cltctl c join cltdmo d on c.clt_num = d.clt_num and d.population = 'mi adult'
join v_autsvc a on c.clt_num = a.clt_num
and a.autsts = 1
and getdate() between a.autbeg_dte and a.autend_dte
and a.proc_cde in ('90862','90801')
join facemp f on d.priemp_num = f.facemp_num
left join phy p on d.pspemp_num = p.phy_num
left join #PrimaryService ps on ps.clt_num = c.clt_num
left join (select clt_num, count(*) as num_claims
from v_clmsvc
where beg_dte between '10/1/2005' and '9/30/2006 23:59'
group by clt_num) n on c.clt_num = n.clt_num
left join #PrimaryLOS los on c.clt_num = los.clt_num
order by c.clt_num

select * from #FinalResults
/*
select * from #FinalResults
where PrimaryService is null

select * from #FinalResults
where LocationOfService is null

select * from #FinalResults
where PrimaryPsychiatrist is null

select * from #FinalResults
where PrimaryClinician is null

select * from #FinalResults
where num_claims = 0

*/

edited to make word wrap work

blindman
Master Smack Fu Yak Hacker

2365 Posts

Posted - 2007-02-23 : 22:08:32
Why do you start out by dropping #ConsumerProcCodes? You should drop your temporary tables at the end of the procedure if anywhere. They'll be automatically dropped when your connection finishes anyway, so issuing the drops at the start of your code is just going to generate pointless error messages. Heck, you would be better off creating them as table variables than as temp tables.

STAR SCHEMAS ARE NOT DATA WAREHOUSES!
Go to Top of Page

Jeff Moden
Aged Yak Warrior

652 Posts

Posted - 2007-02-25 : 15:09:09
[code]Heck, you would be better off creating them as table variables than as temp tables.[/code]

Check out Q3 and Q4 in the following...

http://support.microsoft.com/default.aspx?scid=kb;en-us;305977&Product=sql2k



--Jeff Moden
Go to Top of Page

eyechart
Master Smack Fu Yak Hacker

3575 Posts

Posted - 2007-02-25 : 17:27:19
as blindman suggested, I would get rid of all the drop #temptable statements. Drop them at the end so you don't have any errors when the code executes.

Also, it looks as if some of these temp tables could actually be created as views outside of your stored proc. #ProcCde_Precedence seems to be a temp table that never gets updated and is just used a join in a couple of spots.

Use temp tables and table variables when you need to set aside some data and update the values, or you join on it and the join would be too expensive if a view or subquery was used in place of a temp table. My rule of thumb is to use a set-based approach as much as possible - by createing views, or using subqueries and derived tables. When that isn't possible use a table variable, unless you are dealing with a large dataset or need to create the table from the resultset of a query or stored proc execution. Always avoid cursors (as you have done).



-ec
Go to Top of Page

LaurieCox

158 Posts

Posted - 2007-02-26 : 08:28:41
Hi all,

Thanks for the responses.

Re Drop tables: I am not turning this into a stored procedure. I am just running this straight from query analyzer. The drop statement is there so that when I was working on the temp tables I could easily rerun the create statement. I would just select both the drop and create statements and then hit the execute button.

If I were going to turn it into an SP I would drop the drop tables. I would also use table variables in an SP. But when doing it interactive it was easier to modify, test and run the final query with temp tables. I can just run the final query with out rerunning the other queries.

Is there anything else that I could do better or more elegant?

Thanks,

Laurie
Go to Top of Page

harsh_athalye
Master Smack Fu Yak Hacker

5581 Posts

Posted - 2007-02-26 : 08:35:06
Yeah, few more points:

1. Convert all those SELECT...INTO statements into CREATE TABLE and INSERT INTO...SELECT dialect.
2. Convert IN queries to use EXISTS operator
3. Change date criterias to use ISO date format

Harsh Athalye
India.
"The IMPOSSIBLE is often UNTRIED"
Go to Top of Page
   

- Advertisement -