Spaghetti Code? - SQL Server Forums

Please start any new threads on our new site at https://forums.sqlteam.com. We've got lots of great SQL Server experts to answer whatever question you can come up with.

All Forums

General SQL Server Forums

New to SQL Server Programming

Spaghetti Code?

Author

Topic

GregDDDD
Posting Yak Master

120 Posts

Posted - 2011-06-10 : 12:55:59

I have a situation where I need to process new rows based on 1 of 6 status messages in the row. Four of the types will be processed almost identically, and the last 2 need unique handling.

Normally in VB.Net or C# I would do something like....


If msgStatus = 'Pended' OR msgStatus = 'Rejected' OR msgStatus = 'Canceled'  OR msgStatus = 'Expired'  Then

ElseIF msgStatus = 'New' Then

ElseIF msgStatus = 'Accepted' Then

End If

I could not make something like that work for me here. Instead, I used a variable with a CASE...WHEN block and then test the value of the variable with a single IF statement (See below).

My question is, is this something that would make a SQL Programmer cringe? Is there a more eloquent way to do this?


--Potential msgStatus could be 'New', 'Accepted', 'Pended', 'Rejected', 'Expired', 'Cancel'
DECLARE @msgStatus varchar(10)
set @msgStatus = (SELECT [MsgStatus] FROM [DBName].[dbo].[ReferralsIn] WHERE IndexID = @thisItem)

IF @msgStatus IS NULL
	BEGIN
		--Send a message that there is a problem with @thisItem
		RETURN
	END
	
DECLARE @ProcessCancelEvent int
SET @ProcessCancelEvent = CASE 
							WHEN @msgStatus = 'Cancel' THEN 1
							WHEN @msgStatus = 'Rejected' THEN 1
							WHEN @msgStatus = 'Expired' THEN 1
							WHEN @msgStatus = 'Pended' THEN 1
							ELSE 0
						   END

IF @ProcessCancelEvent = 1
	BEGIN
		--Process all 'Pended', 'Rejected', 'Expired', 'Cancel' items here
	END
	
IF @msgStatus = 'New'
	BEGIN
		--Process 'New' items here
	END

IF @msgStatus = 'Accepted'
	BEGIN
		--Process 'Accepted' items here
	END

Seventhnight
Master Smack Fu Yak Hacker

2878 Posts

Posted - 2011-06-10 : 14:04:25

Ideally, a single set-based solution is preferred.

However, depending on the 'Processing' involved in each case, it may not be appropriate to try and force them together.

If this in a stored procedure, you might run into plan caching problems (especially if a particular case is ALOT more frequent than the others)

You could create individual procedures for each case and use a separate sp to direct the traffic... but that's typically overkill.

It all depends on the specifics.

Corey

I Has Returned!!

GregDDDD
Posting Yak Master

120 Posts

Posted - 2011-06-10 : 14:14:04

By 'single set-based solution' do you mean a separate block for each msgStatus type? That was the way I was headed but I found I was creating the same code for the 4 that are nearly identical. The only difference is one word written to a field.

The processing involved is lite. When a 'New' event happens I append a single row to a table. With 'Accept' I update 4 fields in that row. With the other 4 I am modifying 4 different fields than for 'Accept', with one field getting either 'Pended', 'Rejected', 'Expired', or 'Cancel'. I suppose I could do 4 IF blocks and call a single sp and pass in either 'Pended', 'Rejected', 'Expired', or 'Cancel'.

Seventhnight
Master Smack Fu Yak Hacker

2878 Posts

Posted - 2011-06-10 : 15:58:40

What I meant, was a single query. Well, maybe 2 queries (insert & update).

If you're processing is really light.. try a couple of queries

#1
Insert Into table
Select ...New Records

#2 - Acccept
Update table
Set
field1 = 'value1',
field2 = 'value2',
field3 = 'value3',
field4 = 'value4'
From table
Where IndexId = @thisItem
and @msgStatus = 'New'

#3 - Not New & Not Accept
Update table
Set
field5 = @msgStatus ,
field6 = 'value6',
field7 = 'value7',
field8 = 'value8'
From table
Where IndexId = @thisItem
and @msgStatus in ('Cancel','Rejected','Expired','Pended')

Corey

I Has Returned!!

GregDDDD
Posting Yak Master

120 Posts

Posted - 2011-06-10 : 18:06:47

Yes, if you look at the first post that is what I have. I guess the problem I'm asking about is creating the logic to determine when to fire which of the 3 queries. Do I create 3 different stored procedures and then have 6 different IF...BEGIN...END blocks, with 4 of them calling the same sp and passing the msgStatus, or is doing it the way I did it in my example acceptable. If both work, then what would a seasoned SQL Programmer do? Does it matter? Would a seasoned programmer look and my code and say to themselves, "This guy doesn't know what he's doing".

jcelko
Esteemed SQL Purist

547 Posts

Posted - 2011-06-10 : 21:54:59

People cannot read your mind, so post your code and clear specs if you really want help. Please post real DDL and not narrative or your own personal programming language. Learn to use ISO-11179 rules for the data element names, avoid needless dialect and use ISO-8601 temporal formats, codes and so forth. Please tell us what SQL product and release you are using. Tell us if you can change the DDL or if you are stuck with it.

>> I have a situation where I need to process new rows based on 1 of 6 status messages in the row. Four of the types will be processed almost identically, and the last 2 need unique handling. Normally in VB.Net or C# I would do something like.. <<

In the SQL we hate procedural code like this; SQL is declarative. Totally different mindset. Oh, nobody uses camel case any more. It makes the eyes jump to the uppercase letter then back to the start. SQL has ISO-11179 rules.

You are still thinking that CASE is a control flow construct; it is not. It is an expression.

IF message_status = 'PENDED' OR message_status = 'REJECTED' OR message_status = 'CANCELED' OR message_status = 'EXPIRED'
THEN ELSEIF mesage_status = 'NEW'
THEN ELSEIF message_status = 'ACCEPTED' THEN END IF

>> My question is, is this something that would make a SQL Programmer cringe? Is there a more eloquent way to do this? <<

Cringing. We do not use local variables; we nest expressions. Try this:

IF (SELECT message_status
FROM Referrals
WHERE index_id = @in_index_id) IS NULL
BEGIN
--Send a message that there is a problem with @in_index_id
END;

But the DDL would make this impossible by requiring that there is always a message_status. SQL tables are not like files; the data has constraints, which is why columns qre not anything like fields.

Next, we have shorthand for predicates, so you can use an IN().

IF CASE WHEN message_status IN ('pended', 'rejected', 'canceled', 'expired')
THEN 1 ELSE 0 END = 1
BEGIN
--Process all 'Pended', 'Rejected', 'Expired', 'Cancel' items here
END
ELSE IF msg_status = 'New'
BEGIN
--Process 'New' items here
END
ELSE IF msg_status = 'Accepted'
BEGIN
--Process 'Accepted' items here
END;

If we see the insides of the process, we can do more.

--CELKO--
Books in Celko Series for Morgan-Kaufmann Publishing
Analytics and OLAP in SQL
Data and Databases: Concepts in Practice
Data, Measurements and Standards in SQL
SQL for Smarties
SQL Programming Style
SQL Puzzles and Answers
Thinking in Sets
Trees and Hierarchies in SQL

jcelko
Esteemed SQL Purist

547 Posts

Posted - 2011-06-10 : 22:06:46

>> When a 'New' event happens I append a single row to a table. <<

No, SQL will insert. we do not append because tables have no ordering. Starting to see the mindset? Use the right terms and you have won half the battle.

>>< With 'Accept' I update 4 fields [sic] in that row. With the other 4 I am modifying 4 different fields [sic] than for 'Accept', with one fie3ld [sic'] getting either 'WHEN NOT MATCHED
THEN INSERT VALUES (..)
. I suppose I could do 4 IF blocks and call a single sp and pass in either 'Pended', 'Rejected', 'Expired', or 'Cancel'. <<

This will be a single EMERGE statement:

MERGE INTO..
USING..
ON ..
WHEN NOT MATCHED
THEN INSERT VALUES (..)
WHEN MATCHED
THEN UPDATE
SET col1
= CASE WHEN .. THEN ..
WHEN .. THEN ..
WHEN .. THEN ..
ELSE col1 END, –- do nothing

col2
= CASE WHEN .. THEN ..
WHEN .. THEN ..
WHEN .. THEN ..
ELSE col2 END, –- do nothing
col3
= CASE WHEN .. THEN ..
WHEN .. THEN ..
WHEN .. THEN ..
ELSE col3 END, –- do nothing
col4
= CASE WHEN .. THEN ..
WHEN .. THEN ..
WHEN .. THEN ..
ELSE col4 END; –- do nothing

--CELKO--
Books in Celko Series for Morgan-Kaufmann Publishing
Analytics and OLAP in SQL
Data and Databases: Concepts in Practice
Data, Measurements and Standards in SQL
SQL for Smarties
SQL Programming Style
SQL Puzzles and Answers
Thinking in Sets
Trees and Hierarchies in SQL

GregDDDD
Posting Yak Master

120 Posts

Posted - 2011-06-12 : 16:24:56

jcelko, thanks for the help. Yes, I am getting some of the terminology wrong, but that is why I post to the "New to SQL Server Programming" forum. I will take what you have written to heart and rewrite my SQL code. Programming in SQL Server is very different from what I'm accustomed to, but I am learning.

The one exception I would take with your comments is when you wrote "Oh, nobody uses camel case any more". You can't find a C# or Java programmer who doesn't use it, only now it is first word all lower case and following words all capitalized. I have always declared my variables like CamelCaseVarName, but now it would be camelCaseVarName. As I drift in to programming more in Java and C# I've taken to declaring my variables as "camelCaseVarName" even though I'm not really sure why they don't capitalize the first letter. I guess there is that need to want to fit in. So now I'll need to use 4 different styles of declaring variables depending upon the language I'm writing in.

SQL Server = camel_case_var_name
VB.Net = CamelCaseVarName
C# & Java = camelCaseVarName
ASP.Net = camelcasevarname

You'll notice in my first post I have a mix of styles.

GregDDDD
Posting Yak Master

120 Posts

Posted - 2011-06-13 : 11:31:45

quote:
We do not use local variables; we nest expressions. Try this:

I can see the benefit to not using local variables in some cases. For instance, your example below it is a much more eloquent way of achieving the same thing. The way I wrote it, the local variable was only used once.


IF (SELECT message_status 
      FROM Referrals
      WHERE index_id = @in_index_id) IS NULL
        BEGIN
        --Send a message that there is a problem with @in_index_id
        END;

What about in the case of my use of @thisItem or @msgStatus where they are used repeatedly in the script. I'm not done with this yet and I've already used @thisItem 34 times. Would I still be expected to call a sub query 34 times to use this value, or is storing it in a local variable acceptable.

Subscribe to SQLTeam.com

SQLTeam.com Articles via RSS

SQLTeam.com Weblog via RSS

- Advertisement -

Resources