6
\$\begingroup\$

I have not written very many stored procedures before, and this is the first one I have written to be used in a real business application to write new data into the database, so I would like to get advice to improve it from more experienced SQL developers.

In short it is meant to insert a new person along with their contact information, if it is made available. Several fields allow null because the information is not necessarily required for our purposes.

I have tested all the data validations to make sure errors are thrown when validations fail.

For reference, this is using version Microsoft SQL Server 2014.

Here is the stored procedure create query for review:

if exists (
    select 
        * 
    from 
        INFORMATION_SCHEMA.ROUTINES
    where
        ROUTINE_NAME = 'usp_CreatePerson'
        and SPECIFIC_SCHEMA = 'PsychoProductions'
        and ROUTINE_TYPE = 'PROCEDURE'
)
drop procedure PsychoProductions.usp_CreatePerson;
go

create procedure PsychoProductions.usp_CreatePerson
    @PersonRoleId int,
    @Name varchar(500) = null,
    @Organization varchar(500) = null,
    @Website varchar(500) = null,
    @DefaultBillingMethodId int = 1, --unassigned
    @AddressTypeId int = null,
    @StreetAddress varchar(500) = null,
    @City varchar(200) = null,
    @State char(2) = null,
    @Zip varchar(10) = null,
    @PhoneTypeId int = null,
    @PhoneNumber varchar(20) = null,
    @EmailTypeId int= null,
    @EmailAddress varchar(200) = null
as
set nocount on;

begin transaction;

if not exists (select 1 from PsychoProductions.PersonRoles where Id = @PersonRoleId)
begin;
    throw 50001, 'Invalid PersonRoleId', 1
    rollback transaction;
end;
else 
begin;
    insert into PsychoProductions.Persons (
        PersonRoleId,
        Name,
        Organization,
        Website,
        DefaultBillingMethodId,
        IsActive
    ) values (
        @PersonRoleId,
        @Name,
        @Organization,
        @Website,
        @DefaultBillingMethodId,
        1
    );

    declare @NewPersonId int = @@IDENTITY;

    if @AddressTypeId is not null
    begin;
        if not exists (select 1 from PsychoProductions.AddressTypes where Id = @AddressTypeId)
        begin;
            throw 50001, 'Invalid AddressTypeId', 1;
            rollback transaction;
        end;
        else 
        begin;
            insert into PsychoProductions.PersonAddresses (
                PersonId,
                AddressTypeId,
                StreetAddress,
                City,
                St,
                Zip
            ) values (
                @NewPersonId,
                @AddressTypeId,
                @StreetAddress,
                @City,
                @State,
                @Zip
            );
        end;
    end;

    if @PhoneTypeId is not null
    begin;
        if not exists (select 1 from PsychoProductions.PhoneTypes where Id = @PhoneTypeId)
        begin;
            throw 50001, 'Invalid PhoneTypeId', 1;
            rollback transaction;
        end;
        else
        begin;
            insert into PsychoProductions.PersonPhones (
                PersonId,
                PhoneTypeId,
                PhoneNumber
            ) values (
                @NewPersonId,
                @PhoneTypeId,
                @PhoneNumber
            );
        end;
    end;

    if @EmailTypeId is not null
    begin;
        if not exists (select 1 from PsychoProductions.EmailTypes where Id = @EmailTypeId)
        begin;
            throw 50001, 'Invalid EmailTypeId', 1;
            rollback transaction;
        end;
        else
        begin;
            insert into PsychoProductions.PersonEmails (
                PersonId,
                EmailTypeId,
                Email
            ) values (
                @NewPersonId,
                @EmailTypeId,
                @EmailAddress
            );
        end;
    end;
    commit transaction;
end;

go

Here is an example valid procedure call:

DECLARE @RC int
DECLARE @PersonRoleId int = 2
DECLARE @Name varchar(500) = 'John Doe'
DECLARE @Organization varchar(500) = 'John Doe Inc'
DECLARE @Website varchar(500) = null
DECLARE @DefaultBillingMethodId int = 1
DECLARE @AddressTypeId int = 1
DECLARE @StreetAddress varchar(500) = '123 Main Street'
DECLARE @City varchar(200) = 'New York'
DECLARE @State char(2) = 'NY'
DECLARE @Zip varchar(10) = '12345'
DECLARE @PhoneTypeId int = 1
DECLARE @PhoneNumber varchar(20) = '(111)111-1111'
DECLARE @EmailTypeId int = 1
DECLARE @EmailAddress varchar(200) = 'john@johndoeinc.biz'

EXECUTE @RC = [PsychoProductions].[usp_CreatePerson] 
   @PersonRoleId
  ,@Name
  ,@Organization
  ,@Website
  ,@DefaultBillingMethodId
  ,@AddressTypeId
  ,@StreetAddress
  ,@City
  ,@State
  ,@Zip
  ,@PhoneTypeId
  ,@PhoneNumber
  ,@EmailTypeId
  ,@EmailAddress

Results:

Id          PersonRoleId Name     Organization Website     DefaultBillingMethodId IsActive    CreatedDate
----------- ------------ -------- ------------ ----------- ---------------------- ----------- -----------------------
10          2            John Doe John Doe Inc NULL        1                      1           2016-06-17 17:35:56.813
Id          PersonId    AddressTypeId StreetAddress   City     St   Zip   CreatedDate
----------- ----------- ------------- --------------- -------- ---- ----- -----------------------
2           10          1             123 Main Street New York NY   12345 2016-06-17 17:35:56.812
Id          PersonId    PhoneTypeId PhoneNumber   CreatedDate
----------- ----------- ----------- ------------- -----------------------
6           10          1           (111)111-1111 2016-06-17 17:35:56.813
Id          PersonId    EmailTypeId Email               CreatedDate
----------- ----------- ----------- ------------------- -----------------------
5           10          1           john@johndoeinc.biz 2016-06-17 17:35:56.813

For reference, the table structure for these tables looks like this:

create table PsychoProductions.Persons (
    Id int identity(1,1) primary key,
    PersonRoleId int not null 
        references PsychoProductions.PersonRoles(Id),
    Name varchar(500) null,
    Organization varchar(500) null,
    Website varchar(500) null,
    DefaultBillingMethodId int not null
        references PsychoProductions.BillingMethods(Id),
    IsActive bit default 1,
    CreatedDate datetime default getdate()
);

create table PsychoProductions.PersonAddresses (
    Id int identity(1,1) primary key,
    PersonId int not null
        references PsychoProductions.Persons(Id),
    AddressTypeId int not null
        references PsychoProductions.AddressTypes(Id),
    StreetAddress varchar(500) null,
    City varchar(200) null,
    St char(2) null,
    Zip varchar(10) null,
    CreatedDate datetime default getdate()
);

create table PsychoProductions.PersonPhones (
    Id int identity(1,1) primary key,
    PersonId int not null
        references PsychoProductions.Persons(Id),
    PhoneTypeId int not null
        references PsychoProductions.PhoneTypes(Id),
    PhoneNumber varchar(20) not null,
    CreatedDate datetime default getdate()
);

create table PsychoProductions.PersonEmails (
    Id int identity(1,1) primary key,
    PersonId int not null
        references PsychoProductions.Persons(Id),
    EmailTypeId int not null
        references PsychoProductions.EmailTypes(Id),
    Email varchar(200) not null,
    CreatedDate datetime default getdate()
);
\$\endgroup\$
  • \$\begingroup\$ Be aware that reading new identity values can be problematic in some circumstances. \$\endgroup\$ – Michael Green Jun 18 '16 at 9:47
  • 1
    \$\begingroup\$ Have you tested your error handling code? You throw is going to stop processing within the proc leaving the rollback tran to not be fired (until the connection is lost) \$\endgroup\$ – billinkc Jun 20 '16 at 1:54
  • \$\begingroup\$ @billinkc I have tested the error handling as far as whether the error was thrown, I did not consider that it could keep the transaction open until the connection is lost, would you suggest to rollback tran before throwing the exception? \$\endgroup\$ – Phrancis Jun 20 '16 at 2:04
  • 1
    \$\begingroup\$ Yup, be explicit with your code and the future maintainers will thank you. Clean up your mess prior to hitting the eject button \$\endgroup\$ – billinkc Jun 20 '16 at 2:18
  • \$\begingroup\$ Duly noted. If you have a few more things to say along with that, feel welcome to write a short answer! \$\endgroup\$ – Phrancis Jun 20 '16 at 2:27
3
\$\begingroup\$

Per billinkc in the comments, rollback before throw would do what you intend.

Per Michael Green, avoid using @@IDENTITY where possible. I recommend using output instead. Here's an example:

-- ... blah blah blah ...

-- This table will store the Id of the person you inserted
declare @PersonId table (Id int)

-- ... blah blah blah ...

insert into PsychoProductions.Persons (
    PersonRoleId,
    Name,
    Organization,
    Website,
    DefaultBillingMethodId,
    IsActive
) 
output Inserted.Id into @PersonId (Id)
values (
    @PersonRoleId,
    @Name,
    @Organization,
    @Website,
    @DefaultBillingMethodId,
    1
);

-- ... blah blah blah ...

Now you've got the table @PersonId with a single column Id and a single row with the new ID you just inserted, so when you need to access that ID when inserting addresses and phone numbers, you can join or subquery @PersonId like you would any other table.

As far as I'm aware, variable scoping doesn't matter in SQL Server stored procedures – if you declare @PesronId inside the if block, it will still be accessible once the code has left that block. So you could declare the table right before the Persons insert to keep its use obvious, or near the top of the query like normal coding languages, to imply it is used not only in that if block.

Everything else looks good to me.

\$\endgroup\$
  • 2
    \$\begingroup\$ Thank you, I was afraid no one would ever answer this question after 18 months! \$\endgroup\$ – Phrancis Feb 2 '18 at 2:25
  • 1
    \$\begingroup\$ @Phrancis the unanswered queue is a pathway to many abilities some consider to be unnatural... \$\endgroup\$ – Kris Lawton Feb 2 '18 at 8:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.