Another way of coding this stored proc

Camillia
Camillia used Ask the Experts™
on
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

Comment
Watch Question

Do more with

Expert Office
EXPERT OFFICE® is a registered trademark of EXPERTS EXCHANGE®
Phillip BurtonDirector, Practice Manager and Computing Consultant
Awarded 2014
Top Expert 2014

Commented:
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.
Vitor MontalvãoMSSQL Senior Engineer
Distinguished Expert 2017

Commented:
Since each select is querying it's own table I can't see another way to do that.
You dont think I need to create another table and put those letters in there and read the letters from that table??
Should you be charging more for IT Services?

Do you wonder if your IT business is truly profitable or if you should raise your prices? Learn how to calculate your overhead burden using our free interactive tool and use it to determine the right price for your IT services. Start calculating Now!

Vitor MontalvãoMSSQL Senior Engineer
Distinguished Expert 2017

Commented:
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.
Phillip BurtonDirector, Practice Manager and Computing Consultant
Awarded 2014
Top Expert 2014

Commented:
That might be more complicated that your current solution - what's the advantage?
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.
Jim HornSQL Server Data Dude
Most Valuable Expert 2013
Author of the Year 2015

Commented:
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.
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.
Phillip BurtonDirector, Practice Manager and Computing Consultant
Awarded 2014
Top Expert 2014

Commented:
Which is what I was indicating by "as long as it means something to the originator".
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.
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
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 (?)
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

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
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

Scott PletcherSenior DBA
Most Valuable Expert 2018
Top Expert 2014
Commented:
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
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.
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
oh, Walter, yes, I can do that! thanks

Do more with

Expert Office
Submit tech questions to Ask the Experts™ at any time to receive solutions, advice, and new ideas from leading industry professionals.

Start 7-Day Free Trial