August 6, 2008 at 12:01 pm
Please critique this Stored procedure.
I am not new at writing them but this one *should* be easy and I'm not really sure if I'm doing everything right.
IF EXISTS (SELECT * FROM sys.objects
WHERE object_id = OBJECT_ID(N'[dbo].[sp_AddColumn]')
AND type in (N'P', N'PC'))
DROP PROCEDURE [dbo].[sp_AddColumn]
GO
-- ==================================================================
-- Author: MrBaseball34
-- Date: 08/06/2008
--
-- This stored procedure ADDS the column @ColumnName to
-- Table @TableName in DB @DbName
--
-- Parameters:
-- @DBName = Database containing table @TableName
-- @TableName = Table containing column @ColumName
-- @ColumnName = Column to drop
-- @DataType = Datatype for column
-- @Nullable = If column allows null values
-- @Identity = if column is Identity Column
--
-- Usage:
-- (adds an int column that allows NULLS)
-- sp_AddColumn('MyDB', 'MyTable', 'MyColumn', 'int', TRUE, FALSE)
-- (adds an indentity column)
-- sp_AddColumn('MyDB', 'MyTable', 'MyColumn', 'int', FALSE, TRUE)
-- (adds an varchar column that allows NULLS)
-- sp_AddColumn('MyDB', 'MyTable', 'MyColumn', 'varchar(50)', TRUE, FALSE)
-- ==================================================================
CREATE PROCEDURE sp_AddColumn(@DBName sysname
, @TableName sysname
, @ColumnName sysname
, @DataType varchar (50)
, @Nullable Boolean
, @Identity Boolean)
AS
BEGIN
DECLARE @sql nvarchar(max),
@SFound int
SET @SFound = (SELECT COUNT(*) FROM information_schema.columns
WHERE table_catalog = @DBName
AND table_name = @TableName
AND column_name = @ColumnName)
IF(@SFound = 0)
BEGIN
SET @sql = 'ALTER TABLE [' + @TableName + '] ADD [' + @ColumnName + '] [' + @DataType + ']'
IF (@Identity)
BEGIN
SET @sql = @sql + ' IDENTITY(1,1) '
END
IF (@NULLABLE) OR (NOT @Identity)
BEGIN
END
ELSE
BEGIN
SET @sql = @sql + ' NOT NULL '
END
EXECUTE (@SQL)
END
END
August 6, 2008 at 12:20 pm
Here are a couple of comments. Instead of:
DECLARE @sql nvarchar(max),
@SFound int
SET @SFound = (SELECT COUNT(*) FROM information_schema.columns
WHERE table_catalog = @DBName
AND table_name = @TableName
AND column_name = @ColumnName)
IF(@SFound = 0)
It would be more efficient to do this:
If NOT Exists(SELECT * FROM information_schema.columns
WHERE table_catalog = @DBName
AND table_name = @TableName
AND column_name = @ColumnName)
Secondly you need to realize that INFORMATION_SCHEMA.COLUMNS is bound to database in which you are currently in, so if you run this procedure from master it will return no rows for any other database passed in the @DBName variable, so that variable really gets you nothing unless unless you use dynamic sql like this:
Declare @sql nvarchar(max), @params nvarchar(100), @Rows Int
Set @sql = 'SELECT @Rows = COunt(*) FROM ' + @DBNAME + '.information_schema.columns Where table_name = @TableName AND column_name = @ColumnName'
Set @params = '@Rows Int Output, @TableName varchar(100), @ColumnName varchar(100)'
Exec sp_executesql @sql, @params, @TableName = @TableName, @ColumnName = @ColumnName, @Rows = @Sfound Output
Otherwise I think it will work.
Jack Corbett
Consultant - Straight Path Solutions
Check out these links on how to get faster and more accurate answers:
Forum Etiquette: How to post data/code on a forum to get the best help
Need an Answer? Actually, No ... You Need a Question
August 6, 2008 at 12:25 pm
I would change the SFound thing to an If Exists or If Not Exists. It should, however, work as written, and this is a minor change and not critical.
The other thing to keep in mind is, what will this proc be used for? If it's exposed to public use, it opens your database up completely to SQL injection attacks. If it's for the DBA, why even build it when it'll be just as easy to add columns by typing the command directly? If it's some automation thing and will never been used by a human being directly, then it should be fine.
- Gus "GSquared", RSVP, OODA, MAP, NMVP, FAQ, SAT, SQL, DNA, RNA, UOI, IOU, AM, PM, AD, BC, BCE, USA, UN, CF, ROFL, LOL, ETC
Property of The Thread
"Nobody knows the age of the human race, but everyone agrees it's old enough to know better." - Anon
Viewing 3 posts - 1 through 2 (of 2 total)
You must be logged in to reply to this topic. Login to reply