SolvedPrivate

Another way of coding this stored proc

Posted on 2014-12-01
19
34 Views
Last Modified: 2016-02-18
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
Comment
Question by:Camillia
  • 9
  • 3
  • 3
  • +3
19 Comments
 
LVL 24

Expert Comment

by:Phillip Burton
ID: 40473633
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
 
LVL 45

Expert Comment

by:Vitor Montalvão
ID: 40473640
Since each select is querying it's own table I can't see another way to do that.
0
 
LVL 7

Author Comment

by:Camillia
ID: 40473654
You dont think I need to create another table and put those letters in there and read the letters from that table??
0
 
LVL 45

Expert Comment

by:Vitor Montalvão
ID: 40473663
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
 
LVL 24

Expert Comment

by:Phillip Burton
ID: 40473665
That might be more complicated that your current solution - what's the advantage?
0
 
LVL 7

Author Comment

by:Camillia
ID: 40473685
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
 
LVL 65

Expert Comment

by:Jim Horn
ID: 40473690
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
 
LVL 7

Author Comment

by:Camillia
ID: 40473709
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
 
LVL 24

Expert Comment

by:Phillip Burton
ID: 40473713
Which is what I was indicating by "as long as it means something to the originator".
0
How your wiki can always stay up-to-date

Quip doubles as a “living” wiki and a project management tool that evolves with your organization. As you finish projects in Quip, the work remains, easily accessible to all team members, new and old.
- Increase transparency
- Onboard new hires faster
- Access from mobile/offline

 
LVL 7

Author Comment

by:Camillia
ID: 40473715
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
 
LVL 10

Accepted Solution

by:
Walter Padrón earned 250 total points
ID: 40473796
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
 
LVL 7

Author Comment

by:Camillia
ID: 40473870
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
 
LVL 7

Author Comment

by:Camillia
ID: 40473885
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
 
LVL 10

Expert Comment

by:Walter Padrón
ID: 40473998
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
 
LVL 7

Author Comment

by:Camillia
ID: 40474023
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
 
LVL 69

Assisted Solution

by:ScottPletcher
ScottPletcher earned 250 total points
ID: 40474035
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
 
LVL 7

Author Comment

by:Camillia
ID: 40474052
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
 
LVL 10

Expert Comment

by:Walter Padrón
ID: 40474060
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
 
LVL 7

Author Comment

by:Camillia
ID: 40474065
oh, Walter, yes, I can do that! thanks
0

Featured Post

Enabling OSINT in Activity Based Intelligence

Activity based intelligence (ABI) requires access to all available sources of data. Recorded Future allows analysts to observe structured data on the open, deep, and dark web.

Join & Write a Comment

Calculating holidays and working days is a function that is often needed yet it is not one found within the Framework. This article presents one approach to building a working-day calculator for use in .NET.
Use this article to create a batch file to backup a Microsoft SQL Server database to a Windows folder.  The folder can be on the local hard drive or on a network share.  This batch file will query the SQL server to get the current date & time and wi…
Familiarize people with the process of retrieving data from SQL Server using an Access pass-thru query. Microsoft Access is a very powerful client/server development tool. One of the ways that you can retrieve data from a SQL Server is by using a pa…
Via a live example, show how to setup several different housekeeping processes for a SQL Server.

746 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

11 Experts available now in Live!

Get 1:1 Help Now