Want to protect your cyber security and still get fast solutions? Ask a secure question today.Go Premium

x
  • Status: Solved
  • Priority: Medium
  • Security: Private
  • Views: 56
  • Last Modified:

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

0
Camillia
Asked:
Camillia
  • 9
  • 3
  • 3
  • +3
2 Solutions
 
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
Concerto's Cloud Advisory Services

Want to avoid the missteps to gaining all the benefits of the cloud? Learn more about the different assessment options from our Cloud Advisory team.

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

Featured Post

[Webinar] Database Backup and Recovery

Does your company store data on premises, off site, in the cloud, or a combination of these? If you answered “yes”, you need a data backup recovery plan that fits each and every platform. Watch now as as Percona teaches us how to build agile data backup recovery plan.

  • 9
  • 3
  • 3
  • +3
Tackle projects and never again get stuck behind a technical roadblock.
Join Now