Another way of coding this stored proc

I know there has to be a better way of coding this.

In the ASP.Net code, I need to verify 4 conditions before user can submit their form. I need to display a msg based on these 4 conditions.

So, I wrote this to return a letter back to the code. I don't think this is a good way to just return a letter. Should I create another table with the exact msg and what the letter corresponds to? should I just return true or false?

ALTER Procedure [dbo].[usp_ValidatioSubmission]

@PurchaseDate as datetime,
@SerialNumber as varchar(50),
@ProductId as int,
@PromotionId as int,
@result as varchar(1) Output

as
begin

--1. check if productId, PromotionId and Serial number exist. Invalid submission if it exists
 if exists (select * from consumer where [ProductId] = @productId
                                         And [ProductSerialNumber] = @SerialNumber
										 and [PromotionId] = @PromotionId)
         return 'P' --TODO put something meaningful here
 
 --2 invalid serial number and product combination                                        
 if not exists (select * from [dbo].[Products] where Id = @ProductId 
                                                 and [SerialNumber] = @SerialNumber)
		return 'S'

--3. Is purchaseDate within the promotion
 if not exists (select * from [dbo].[Promotions] where id = @PromotionId 
                                                       and @PurchaseDate between [StartDate] and [EndDate])
        return 'D'

-- 4 product is not in promotion
 If not exists (select * from [dbo].[ProductsInPromotion] where ProductId = @ProductId
                                                                and [PromotionID] = @PromotionId)
			return 'I'													 

end

Open in new window

LVL 8
CamilliaAsked:
Who is Participating?
I wear a lot of hats...

"The solutions and answers provided on Experts Exchange have been extremely helpful to me over the last few years. I wear a lot of hats - Developer, Database Administrator, Help Desk, etc., so I know a lot of things but not a lot about one thing. Experts Exchange gives me answers from people who do know a lot about one thing, in a easy to use platform." -Todd S.

Phillip BurtonDirector, Practice Manager and Computing ConsultantCommented:
It's an OK way to return it, as long as it means something to the originator.

If you want to add auditing, then you can also append to an existing table the time of the search and the result.
0
Vitor MontalvãoMSSQL Senior EngineerCommented:
Since each select is querying it's own table I can't see another way to do that.
0
CamilliaAuthor Commented:
You dont think I need to create another table and put those letters in there and read the letters from that table??
0
Ultimate Tool Kit for Technology Solution Provider

Broken down into practical pointers and step-by-step instructions, the IT Service Excellence Tool Kit delivers expert advice for technology solution providers. Get your free copy now.

Vitor MontalvãoMSSQL Senior EngineerCommented:
Depends. If they are static that you don't need to so you'll save time (less one table to query).
But if something that is always changing then yes. It's better to change a table value than the code.
0
Phillip BurtonDirector, Practice Manager and Computing ConsultantCommented:
That might be more complicated that your current solution - what's the advantage?
0
CamilliaAuthor Commented:
Yeah. I thought I should have a table just in case my manager gets on my case for not understanding those values. Good, I'll leave it as is.
0
Jim HornMicrosoft SQL Server Developer, Architect, and AuthorCommented:
Just to throw it out there ... four tests in the SP, can more than one of these fail, and if so do you want both letters returned?

If so, something like...

Declare @return varchar(10) = ''

IF (test 1 goes here) 
   @return = @return + 'P'
ELSE IF (test 2 goes here) 
   @return = @return + 'S'
ELSE IF (test 3 goes here) 
   @return = @return + 'D'
ELSE IF (test 4 goes here) 
   @return = @return + 'I'

-- Return Set
IF LEN(@return) > 1
   SELECT @return
ELSE 
  -- return whatever you want if there are no validation errors here

Open in new window


Also, a case can be made to make a more English-friendly error message in your SP then using a single letter.
0
CamilliaAuthor Commented:
Also, a case can be made to make a more English-friendly error message in your SP then using a single letter.

Yeah, they letters are not descriptive of the error msgs.
0
Phillip BurtonDirector, Practice Manager and Computing ConsultantCommented:
Which is what I was indicating by "as long as it means something to the originator".
0
CamilliaAuthor Commented:
I made up the letters :) I was coding late last night and couldnt think of anything else. In the ASP.Net code, I'll check for the letters and display a msg to the user.
0
Walter PadrónCommented:
This code works but in the long term it hurts more than good, this is why in IMHO i said that:

- Error-handling is a CROS-CUTTING CONCERN and is DOMAIN specific so it doesn't belongs to the access layer less to the database.  What if you need to check the conditions and you forget to call the sproc?

- If you need more validation logic you added that check to the procedure, and it  will becomes a GOD OBJECT

- Returning a letter doesn't throw for instance an exception so this doesn't guarantee that you catch the errors, your error-handling will be "if" based. And every time you added a return letter to the sproc you need to add an "if" in one or more places of your application.

- If you hard-coded the  validation error strings in the database it will become more difficult to globalize the application

- How do you TEST this logic? This kind of design is difficult to TEST and MAINTAIN in the long term.

So, i shall move all this validation logic to the application

I suggest you to read about Patterns and Design, these are some principles that introduce you to the topic
http://en.wikipedia.org/wiki/SOLID_%28object-oriented_design%29

Best regards
0

Experts Exchange Solution brought to you by

Your issues matter to us.

Facing a tech roadblock? Get the help and guidance you need from experienced professionals who care. Ask your question anytime, anywhere, with no hassle.

Start your 7-day free trial
CamilliaAuthor Commented:
Thanks, Walter. I've read that article before. I'm under a deadline...that's why I used this "hack" (mostly).

I will move the validation to the application but I still need to do those sql statements. I guess I need to do them in code using LINQ (?)
0
CamilliaAuthor Commented:
I rewrote the first one in LINQ, in code. I think it looks better.Might have to change that "true" and "false" to a letter or something else to know what threw error.
   //1. check if productId, PromotionId and Serial number exist 
            var p = (from s in _dataContext.Consumers
                where s.ProductId == model.ProductId &&
                      s.PromotionId == model.PromotionsId &&
                      s.ProductSerialNumber == model.SerialNumber
                select s);

            if (p != null) //row exists
                return false;

Open in new window

0
Walter PadrónCommented:
Yes, i know what is having a deadline. My recomendation is: survive the deadline :) and then REFACTOR

This approach Is better because moved the validation code away from the database. I shall throw an exception with the reason in the inner details (no more letter codes and in the UI just print the reason that decouples the interface from domain logic).

Best regards
0
CamilliaAuthor Commented:
With the way I have it now, I return true or false. I can't display the actual reason to the user (might be ok for now). I'm thinking I have to get back to using a letter to indicate why the form submission was failed.

For example

 var y = (from s in _dataContext.Promotions
                where s.Id == model.PromotionsId &&
                      (model.PurchaseDate >= s.StartDate && model.PurchaseDate <= s.EndDate)
                select s);
            if (!y.Any())
                return false;

Open in new window

0
Scott PletcherSenior DBACommented:
You can't directly return char(s) from a proc in SQL Server, only a single integer value.

But there's nothing wrong with using bit values in that return value to indicate errors.  Errors are exceptions anyway, of course, so I wouldn't waste too much overhead and coding dealing with them.  The original caller could call a standardized error routine if it needed to to translate errors.

If you want go get more sophisticated, I suggest using sp_addmessage to align with SQL Server's own approach.


declare @return_code int
set @return_code = 0

--1. check if productId, PromotionId and Serial number exist. Invalid submission if it exists
 if exists (select * from consumer where [ProductId] = @productId
                                         And [ProductSerialNumber] = @SerialNumber
                                                             and [PromotionId] = @PromotionId)
         set @return_code = @return_code | 1
 
 --2 invalid serial number and product combination                                        
 if not exists (select * from [dbo].[Products] where Id = @ProductId
                                                 and [SerialNumber] = @SerialNumber)
         set @return_code = @return_code | 2

--3. Is purchaseDate within the promotion
 if not exists (select * from [dbo].[Promotions] where id = @PromotionId
                                                       and @PurchaseDate between [StartDate] and [EndDate])
         set @return_code = @return_code | 4

-- 4 product is not in promotion
 If not exists (select * from [dbo].[ProductsInPromotion] where ProductId = @ProductId
                                                                and [PromotionID] = @PromotionId)
         set @return_code = @return_code | 8
         
 if @return_code > 0
     return @return_code
0
CamilliaAuthor Commented:
You're right, Scott. I think I could've done " select 'S' "  and not "return 'S'"

I moved this to the code for now and I'll see how it works.
0
Walter PadrónCommented:
These are only suggestions because we don't know the inners of the project and  refactoring software can be difficult. Do whatever you are confortable with and later REFACTOR

Instead of true or false you can do this because validation frameworks usually catch exceptions
If (!y.Any())
{
    throw new ArgumentException("Reason: Your text should be here...");
}

Open in new window


Best regards
0
CamilliaAuthor Commented:
oh, Walter, yes, I can do that! thanks
0
It's more than this solution.Get answers and train to solve all your tech problems - anytime, anywhere.Try it for free Edge Out The Competitionfor your dream job with proven skills and certifications.Get started today Stand Outas the employee with proven skills.Start learning today for free Move Your Career Forwardwith certification training in the latest technologies.Start your trial today
Microsoft SQL Server

From novice to tech pro — start learning today.

Question has a verified solution.

Are you are experiencing a similar issue? Get a personalized answer when you ask a related question.

Have a better answer? Share it in a comment.