Code Review

I'd like to start a new series that will help you learn some of the techniques I've used in the past.  Some of these techniques are useful, some aren't.  Using SQL is a constant learning process.  You have to be willing to look at your solutions and throw out those ideas that no longer work as well as they used to.  You have to look at other programmers works and find things that work better than your own.  By sharing my old code, I hope to find ideas that still work well, and look for things that don't.

During this process I hope you can learn from my experience and my mistakes.  Let's start with some code from March 14, 2007.  This code would have run on a SQL 2005 server.  I realize you will not have access to the data these samples were built on.  But I'll do my best to explain the code you see below.  This procedure  was written as a check in an ETL process.

DROP PROCEDURE dbo.usp_ACME_CheckADDFile
GO
CREATE PROCEDURE dbo.usp_ACME_CheckADDFile
 @tablename AS VARCHAR(50),
 @fFilename AS VARCHAR(50)
AS
DECLARE @OutputFile AS VARCHAR(500)
DECLARE @Appl AS VARCHAR(4)
DECLARE @SQL AS VARCHAR(7999)
DECLARE @Joins AS VARCHAR(7999)
DECLARE @Whereby AS VARCHAR(7999)
DECLARE @msgbody AS VARCHAR(8000)

/*Testing Only
DECLARE @tablename AS VARCHAR(50), @filename AS VARCHAR(50)
set @filename = 'GCOADD_354_00025.CSA'
set @tableName = 'tmpaddfile'
*/

IF (SELECT COUNT(*) FROM md.dbo.sysobjects WHERE name = 'tmpACMEOutputErrorMsg') >0
DROP TABLE md.dbo.tmpACMEOutputErrorMsg

CREATE TABLE [md].[dbo].[tmpACMEOutputErrorMsg] (
 [Listnum] [char] (10) NULL,
 [ErrorMsg] [char] (200) NULL
) ON [PRIMARY]

--dynamically generated error check
SET @lStrSQL = 'INSERT INTO md.dbo.tmpACMEOutputErrorMsg SELECT vam.listnum, ''MBID matches both Primary and Secondary Key'''
  + '  as ErrorMsg FROM ' + @tablename
  + ' tmp (NOLOCK) LEFT JOIN v_acmembid vam (NOLOCK) ON LTRIM(RTRIM(tmp.matchback_id)) = LTRIM(RTRIM(vam.primary_MBID)) '
  + ' LEFT JOIN v_acmembid vam2 (NOLOCK) ON LTRIM(RTRIM(tmp.matchback_id)) = LTRIM(RTRIM(vam2.secondary_MBID)) '
  + ' WHERE vam.listnum IS NOT NULL AND vam2.listnum IS NOT NULL'

--print @lStrSQL
EXEC (@lStrSQL)

IF (SELECT COUNT(*) FROM md.dbo.tmpACMEOutputErrorMsg ) > 0
BEGIN

 SET @msgbody = 'Errors in ADD File ' + @pStrFileName

 EXEC msdb.dbo.sp_send_dbmail
 @profile_name = 'Automation',
 @recipients = 'shannon.lowder@company.com',
 @copy_recipients = 'alertList@company.com',
 @body = @msgbody,
 @query = 'select * from md.dbo.tmpACMEOutputErrorMsg order by ErrorMsg',
 @subject = 'acme ADD File Check';
END
GO

The first thing I notice when I look at this code is the first parameter,  @tablename AS VARCHAR(50).  When I look down at the dynamically generated error check I recognize right away that this was how I was getting around the fact that SQL 2005 didn't allow you to pass a table as a parameter.  As of SQL 2008, you can do this.  I would call this function right after loading the raw data into @tableName.  I could generate any number of checks, based on business logic I learn through working with business users.

Also notice the DROP and CREATE TABLE statements for md.dbo.tmpACMEOutputErrorMsg.  I'm basically creating a table that will hold all the errors I detect for in the dynamically generated statements.  If there are any errors generated (COUNT(*) > 0), then I will send an email to myself and an alert group.  This email will contain the records from the tmpACMEOutputErrorMsg table.

Pretty simple stuff.  The big thing I would change now is I would pass the table to the procedure, rather than a reference to the table.  This would change all my dynamic code to static code, and would make this procedure far easier to maintain.  Do you have any questions about this code?  Any suggestions for making it better?  Let me know!