September 19, 2005 at 2:21 pm
I have the following stored procedure:
CREATE PROCEDURE update_target @status varchar(1)=NULL,@trg_id numeric(10,0)=NULL, @mseq numeric(5,0)=NULL, @trg_type_code char(1)=NULL, @trg_sub_code varchar(2)=NULL, @alat numeric(6,4)=NULL, @alng numeric(7,4)=NULL, @tlat numeric(6,4)=NULL, @tlng numeric(7,4)=NULL, @trg_date varchar(12)=NULL, @trg_time varchar(2)=NULL, @range numeric(7,2)=NULL, @bearing numeric(5,2)=NULL, @comments varchar(800)=NULL, @altitude numeric(12,12)=NULL, @code as numeric(5,0)=NULL, @identified_code as numeric(5,0)=NULL,@photo as bit=NULL,@video as bit=NULL, @name varchar(35)=NULL, @side varchar(35)=Null, @callsign varchar(35)=Null, @activity varchar(35)=Null, @nation varchar(35)=Null, @vess_type varchar(35)=Null, @fishery_type varchar(35)=Null, @course varchar(35)=Null, @speed varchar(35)=Null AS
SET NOCOUNT ON SET ANSI_NULLS ON
IF @status IS NULL or @status = '' BEGIN --No target status code specified for INSERT, UPDATE, DELETE RETURN (1) END
ELSE --Check to ensure the target code does not already exist in SIS -- Code to ADD the new target
IF @status = 'A' SET @target_datetime = @trg_date + @trg_time IF EXISTS(select * from target where trg_id=@trg_id and mseq=@mseq) RETURN(2) ELSE BEGIN IF EXISTS(select code from target where mseq=@mseq) BEGIN SET @code = (select max(code) from target where mseq=@mseq) + 1 SET @trg_id = (select max(trg_id) from target where mseq=@mseq) + 1 SET @identified_code = @code END ELSE BEGIN SET @code = 1 SET @identified_code = 1 SET @trg_id = (select max(trg_id) from target where mseq=@mseq) + 1 END
INSERT INTO target (code, mseq,trg_id,trg_type_code,trg_sub_code,alat,alng,tlat,tlng,trg_date,range,bearing,comments,altitude) VALUES (@code, @mseq, @trg_id, @trg_type_code, @trg_sub_code,@alat,@alng,@tlat,@tlng,@target_datetime,@range,@bearing,@comments,@altitude)
INSERT INTO target_identified (mseq, trg_id, identified_code, code, detail1, detail2, detail3, detail4, detail5, detail6, detail7, detail8, detail9, photo, video, comments) VALUES (@mseq, @trg_id, @identified_code, @code, @name, @side, @callsign, @activity, @nation, @vess_type, @fishery_type, @course, @speed, @photo,@video, @comments) RETURN (0) END
-- Code to UPDATE a specified target BEGIN IF @status = 'U' IF EXISTS(select * from target where trg_id=@trg_id and mseq=@mseq) UPDATE target set code=@code, mseq=@mseq, trg_id=@trg_id, trg_type_code=@trg_type_code, trg_sub_code=@trg_sub_code, alat=@alat, alng=alng, tlat=@tlat, tlng=@tlng, trg_date=@target_datetime, range=@range, bearing=@bearing, comments=@comments, altitude=@altitude UPDATE target_identified set trg_id=@trg_id, mseq=@mseq, identified_code=@identified_code, code=@code, detail1=@name, detail2=@side, detail3=@callsign, detail4=@activity, detail5=@nation, detail6=@vess_type, detail7=@fishery_type, detail8=@course, detail9=@speed, photo=@photo, video=@video, comments=@comments RETURN (0) END
-- Code to DELETE a specified target BEGIN IF @status = 'D' IF EXISTS(select * from target where trg_id=@trg_id and mseq=@mseq) DELETE FROM photos where trg_id=@trg_id and mseq=@mseq DELETE FROM target_identified where trg_id=@trg_id and mseq=@mseq DELETE FROM target where trg_id=@trg_id and mseq=@mseq RETURN (0) END
--Check for SQL Server Errors IF @@ERROR <> 0 BEGIN RETURN (4) END
ELSE BEGIN --Successful delete RETURN (0) END GO
When I pass in a status (@status) code of 'U', then the first IF statement after "IF @status = 'A'" is evaluated where I would like the code to jump to next part of code to evaulate the next status (IF @status = 'U') and so on until the proper condition is met. However, no matter what value I pass in for @status, it still tries to evaluate the same code everytime and ignores the other conditional statements. What am I doing wrong? I am using the debugger to pass in the parameters.
September 19, 2005 at 2:25 pm
I see two issues - first, it seems you might be missing a begin ... end block for the Else portion of that first conditional statement. Second, not related to your question -- your error checking method will not work, because @@error is changed after each and every statement, and will not "carry" an error code all the way to the end of your procedure. You need to do error checking immediately after (as in THE next statement) any statement that you feel is important to trap.
September 19, 2005 at 2:29 pm
Also, consider that this one proc maybe does too many different things. Your whole flow control issue might go away if you create separate SPs for each operation. It's always a good design practice to make an individual function (any language) do one simple thing well.
September 19, 2005 at 2:41 pm
SET NOCOUNT ON SET ANSI_NULLS ON
IF @status IS NULL or @status = '' BEGIN --No target status code specified for INSERT, UPDATE, DELETE RETURN (1) END
ELSE --Check to ensure the target code does not already exist in SIS -- Code to ADD the new target BEGIN IF @status = 'A' SET @target_datetime = @trg_date + @trg_time IF EXISTS(select * from target where trg_id=@trg_id and mseq=@mseq) RETURN(2) ELSE BEGIN IF EXISTS(select code from target where mseq=@mseq) BEGIN SET @code = (select max(code) from target where mseq=@mseq) + 1 SET @trg_id = (select max(trg_id) from target where mseq=@mseq) + 1 SET @identified_code = @code END ELSE BEGIN SET @code = 1 SET @identified_code = 1 SET @trg_id = (select max(trg_id) from target where mseq=@mseq) + 1 END
INSERT INTO target (code, mseq,trg_id,trg_type_code,trg_sub_code,alat,alng,tlat,tlng,trg_date,range,bearing,comments,altitude) VALUES (@code, @mseq, @trg_id, @trg_type_code, @trg_sub_code,@alat,@alng,@tlat,@tlng,@target_datetime,@range,@bearing,@comments,@altitude)
INSERT INTO target_identified (mseq, trg_id, identified_code, code, detail1, detail2, detail3, detail4, detail5, detail6, detail7, detail8, detail9, photo, video, comments) VALUES (@mseq, @trg_id, @identified_code, @code, @name, @side, @callsign, @activity, @nation, @vess_type, @fishery_type, @course, @speed, @photo,@video, @comments) RETURN (0) END
-- Code to UPDATE a specified target BEGIN IF @status = 'U' IF EXISTS(select * from target where trg_id=@trg_id and mseq=@mseq) UPDATE target set code=@code, mseq=@mseq, trg_id=@trg_id, trg_type_code=@trg_type_code, trg_sub_code=@trg_sub_code, alat=@alat, alng=alng, tlat=@tlat, tlng=@tlng, trg_date=@target_datetime, range=@range, bearing=@bearing, comments=@comments, altitude=@altitude UPDATE target_identified set trg_id=@trg_id, mseq=@mseq, identified_code=@identified_code, code=@code, detail1=@name, detail2=@side, detail3=@callsign, detail4=@activity, detail5=@nation, detail6=@vess_type, detail7=@fishery_type, detail8=@course, detail9=@speed, photo=@photo, video=@video, comments=@comments RETURN (0) END
-- Code to DELETE a specified target BEGIN IF @status = 'D' IF EXISTS(select * from target where trg_id=@trg_id and mseq=@mseq) DELETE FROM photos where trg_id=@trg_id and mseq=@mseq DELETE FROM target_identified where trg_id=@trg_id and mseq=@mseq DELETE FROM target where trg_id=@trg_id and mseq=@mseq RETURN (0) END
END
--Check for SQL Server Errors IF @@ERROR <> 0 BEGIN RETURN (4) END
ELSE BEGIN --Successful delete RETURN (0) END GO
I just put the BEGIN....END (indicated in red above) in the code. Is this where you indicated it should go? If so, I ran the code in the debugger again and it still the same results. I see your point about breaking this up into individual SPs, but the programmers here wanted the login encapsulated into one stored procedure and then just pass in the @status value. Thanks for the note about @@error. Any other suggestions??
September 19, 2005 at 2:47 pm
Not where it should be (or still missing some).
Also please forget the dev and split this into 3 sps. If he still insists, you can always all the sps from that sp so that he can't complain about it.
September 19, 2005 at 3:01 pm
Cory - there are a great many flow control problems in this code... I am having trouble explaining. I think that your BEGIN and END blocks are in the wrong place(s)
Foe example, what is the meaning of this structure? >>
BEGIN
IF @status = 'D'
IF EXISTS(select * from target where trg_id=@trg_id and mseq=@mseq)
DELETE FROM photos where trg_id=@trg_id and mseq=@mseq
DELETE FROM target_identified where trg_id=@trg_id and mseq=@mseq
DELETE FROM target where trg_id=@trg_id and mseq=@mseq
RETURN (0)
END
Unless I am not following correctly, what that actually says is probably not what you want:
BEGIN [this begin statement does not do anything]
IF @status = 'D' then
IF EXISTS(select * from target where trg_id=@trg_id and mseq=@mseq)
DELETE FROM photos where trg_id=@trg_id and mseq=@mseq [ This single delete will execute if both preceeding IFs are true. ]
DELETE FROM target_identified where trg_id=@trg_id and mseq=@mseq [This always executes]
DELETE FROM target where trg_id=@trg_id and mseq=@mseq [This always executes]
RETURN (0) [This always executes]
END [This END does not do anything]
Do you mean to write
if (something is true)
BEGIN
...do things...
END
which brings me back to my previous suggestion. T-SQL is really, really bad for flow control. It's good at set manipulation and ... well ... nothing else :-).
So why not simplfy life with separate procs for each function you need, even if you wrap them like this for your teammates:
create proc wrapper as
...
if @status = 'A' then execute aProcedure
if @status = 'U' then execute uProcedure
if @status = 'Z' then execute zProcedure
September 19, 2005 at 3:35 pm
I apologize for the confusion Merrill. I will explain further:
The @status variable is used to represent, (A)dd, (U)pdate or (D)elete. If the application passes in 'A', then a new record will be added to the relevant tables. And then so on for Update and Delete. I thought that BEGIN...END was supposed to be used to force the execution of code blocks, that's why I use them.
Yes, I did mean to write:
if (something is true)
BEGIN
...do things...
END
but I also though that I would be able to use decent conditional login by means of IF...ELSE statements to satisfy the multiple conditions (Add, Update, Delete). I did not think that T-SQL was weak in terms of conditional logic. It seems that there are a few limitations...correct me if I am wrong. can this still be accomplished in the same stored procedure given the poor logic flow of T-SQL? If so, could you give an example using my code above? I suspect that I will take your suggestion anyways and break this out into individual stored procedures and then have the main calling stored procedure.
September 19, 2005 at 3:47 pm
Hi - yes, it's technically possible to write a procedure like what you are after, and it can be made to work. It just gets quite confusing to read and troubleshoot when you get to a certain number of nested if/else statements :-). I find breaking it down into smaller, discrete pieces (separate SPs or functions) helps make it more legible, and therefore less prone to error.
When I say T-SQL is "weak" in this area, what I mean is not that it doesn't work -- it does -- but that there are few structures available for flow control and logic that you find in other programming languages (like classes/objects, for example, or exceptions, and so on). So if you have to implement complicate flow control logic, it always seems to turn into this spaghetti of CASE or IF / ELSE procedural code.
Because of that, I think best practice is to have simple, very focused stored procedures that do not try to do too many things. If it were me, for example, your one proc would definitely be three, with the choice of which to call placed in the app's court. The app has to choose anyway, in order to set the status you're passing in to begin with.
HTH
🙂
September 19, 2005 at 4:51 pm
Thanks for your help. I would still like to see this work using code similar to my own. Could you give my code a rearrange to show me how this could be done in one stored procedure? Or anyone else reading this post? I would like to se how this could be accomplished in the same stored procedure. Could I use a CASE statement or could this also be accomplished using IF...THEN?
September 19, 2005 at 5:27 pm
This outline should work:
declare @funCode char(1)
set @funCode = 'B'
if @funcode = 'A'
begin
print 'The function code was ''A''.'
end
else if @funcode = 'B'
begin
print 'The function code was ''B''.'
end
else if @funcode = 'C'
begin
print 'The function code was ''C''.'
end
else print 'The function code was not recognised.'
September 20, 2005 at 6:46 am
Ya, that would work. But for the love of G*d, split this into 3 procs and let the app decide, AS IT MUST ANYWAYS.
Viewing 11 posts - 1 through 10 (of 10 total)
You must be logged in to reply to this topic. Login to reply