3
\$\begingroup\$

I am new to the code review area of Stack and welcome all positive criticism.

I would be grateful if someone could review my code below and let me know where I can improve or have gone wrong that I cannot see.

The only thing that I have left is to write the delete and would appreciate help in doing this is possible too please.

My code is

    CREATE TABLE CRM_PERS_CONTACT_INFORMAITON(
 SEQ_ID                     INT IDENTITY(1,1) not null primary key,
 PERS_ID                    INT NOT NULL,
 PREFERRED                  BIT DEFAULT (0), 
 CEASED_DATE                DATETIME,
 COMMUNICATION_TYPE         VARCHAR(50),
 COMMUNICATION_VALUE        VARCHAR(255),
 DATE_ADDED                 DATETIME DEFAULT (GETDATE()),
 ADDED_BY                   VARCHAR(50),
 UPDATED_DATE               DATETIME DEFAULT(GETDATE()),
 UPDATED_BY                 VARCHAR(50)
)
GO

CREATE TABLE CRM_PERS_CONTACT_INFORMAITON_HISTORY(
 SEQ_ID                     INT IDENTITY(1,1) not null primary key,
 PERS_ID                    INT NOT NULL,
 PREFERRED                  BIT DEFAULT (0), 
 CEASED_DATE                DATETIME,
 COMMUNICATION_TYPE         VARCHAR(50),
 COMMUNICATION_VALUE        VARCHAR(255),
 DATE_ADDED                 DATETIME DEFAULT (GETDATE()),
 ADDED_BY                   VARCHAR(50),
 ACTION_TYPE                VARCHAR(20)
 )
 GO

CREATE PROCEDURE MERGE_CRM_PERS_CONTACT_INFORMATION

@SEQ_ID                     INT,
@PERS_ID                    INT ,
@PREFERRED                  BIT , 
@CEASED_DATE                DATETIME,
@DATE_ADDED                 DATETIME,   
@ADDED_BY                   VARCHAR,
@UPDATED_DATE               DATETIME,
@UPDATED_BY                 VARCHAR,
@COMMUNICATION_TYPE         VARCHAR,
@COMMUNICATION_TYPE_VALUE   VARCHAR,
@ACTION_TYPE                VARCHAR

AS
BEGIN

MERGE CRM_PERS_CONTACT_INFORMAITON as Target
USING (select 
 @PERS_ID                   
,@PREFERRED                 
,@CEASED_DATE               
,@DATE_ADDED                    
,@ADDED_BY                  
,@UPDATED_DATE              
,@UPDATED_BY                
,@COMMUNICATION_TYPE        
,@COMMUNICATION_TYPE_VALUE  
) AS Source(
PERS_ID             
,PREFERRED              
,CEASED_DATE            
,DATE_ADDED             
,ADDED_BY               
,UPDATED_DATE           
,UPDATED_BY             
,COMMUNICATION_TYPE     
,COMMUNICATION_TYPE_VALUE
)
ON (Source.PERS_ID = Target.PERS_ID
AND Source.COMMUNICATION_TYPE = Target.COMMUNICATION_TYPE)
WHEN MATCHED THEN 

    update SET 
     COMMUNICATION_TYPE         = @COMMUNICATION_TYPE
    ,COMMUNICATION_VALUE        = @COMMUNICATION_TYPE_VALUE
    ,UPDATED_DATE               = GETDATE()
    ,UPDATED_BY                 = @UPDATED_BY
    ,CEASED_DATE                = @CEASED_DATE
    ,PREFERRED                  = @PREFERRED


WHEN NOT MATCHED BY TARGET THEN
    INSERT (
     PERS_ID            
    ,PREFERRED          
    ,CEASED_DATE        
    ,COMMUNICATION_TYPE
    ,COMMUNICATION_VALUE
    ,DATE_ADDED         
    ,ADDED_BY           
    ,UPDATED_DATE       
    ,UPDATED_BY         
    )
    VALUES (
     @PERS_ID           
    ,@PREFERRED         
    ,@CEASED_DATE       
    ,@COMMUNICATION_TYPE
    ,@COMMUNICATION_TYPE_VALUE
    ,@DATE_ADDED            
    ,@ADDED_BY          
    ,GETDATE()      
    ,@UPDATED_BY            
    )
WHEN NOT MATCHED BY SOURCE THEN 
DELETE;

SET @ACTION_TYPE = $action

INSERT INTO CRM_PERS_CONTACT_INFORMAITON_HISTORY(
 PERS_ID            
,PREFERRED          
,CEASED_DATE        
,COMMUNICATION_TYPE
,COMMUNICATION_VALUE
,DATE_ADDED         
,ADDED_BY
,ACTION_TYPE            
)
VALUES (
 @PERS_ID           
,@PREFERRED         
,@CEASED_DATE       
,@COMMUNICATION_TYPE
,@COMMUNICATION_TYPE_VALUE
,GETDATE()          
,@ADDED_BY
,@ACTION_TYPE
);
END 
GO
\$\endgroup\$
0
\$\begingroup\$

When you declare your variables, like @COMMUNICATION_TYPE VARCHAR, you really need to specify a length.

MSDN

When n is not specified in a data definition or variable declaration statement, the default length is 1. When n is not specified when using the CAST and CONVERT functions, the default length is 30

So go ahead and declare them 20, 50, and 250, respectively, as you did in your table declarations

Additionally, in order to SET @ACTION_TYPE = $action you need to output this from your MERGE() statement.

...
WHEN NOT MATCHED BY SOURCE THEN 
DELETE
OUTPUT $action INTO @ACTION_TYPE;

More commonly MERGE() would be used with a set. That is, you would take in a table variable into your STORED PROCEDURE and your MERGE statement would conduct it's logic on each row. Thus, you could have some rows that were INSERTED, some that were UPDATED, and some that were DELETED. In this instance, you would store the OUTPUT to a table variable or temp table.

CREATE TABLE #MyTempTable(...)
...
MERGE
...
OUTPUT $action, deleted.*, inserted.* INTO #MyTempTable; 

INSERT INTO CRM_PERS_CONTACT_INFORMAITON_HISTORY
SELECT(...) FROM #MyTempTable
\$\endgroup\$
  • \$\begingroup\$ Yes, i just realised that when i was testing it and saw that I was only getting one character. \$\endgroup\$ – user127535 Mar 23 '17 at 16:17
  • \$\begingroup\$ One thing I am having an issue with is the $action I want to determine if its an insert update or delete so that I can update my history table. Are you able to help \$\endgroup\$ – user127535 Mar 23 '17 at 16:18
  • \$\begingroup\$ Sure @SimonPrice I added some information with that. \$\endgroup\$ – scsimon Mar 23 '17 at 16:38

Your Answer

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