?
SolvedPrivate

Another way of coding this stored proc

Posted on 2014-12-01
19
Medium Priority
?
46 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
[X]
Welcome to Experts Exchange

Add your voice to the tech community where 5M+ people just like you are talking about what matters.

  • Help others & share knowledge
  • Earn cash & points
  • Learn & ask questions
  • 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 51

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
Microsoft Certification Exam 74-409

Veeam® is happy to provide the Microsoft community with a study guide prepared by MVP and MCT, Orin Thomas. This guide will take you through each of the exam objectives, helping you to prepare for and pass the examination.

 
LVL 51

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 66

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
 
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 1000 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:Scott Pletcher
Scott Pletcher earned 1000 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

Get 15 Days FREE Full-Featured Trial

Benefit from a mission critical IT monitoring with Monitis Premium or get it FREE for your entry level monitoring needs.
-Over 200,000 users
-More than 300,000 websites monitored
-Used in 197 countries
-Recommended by 98% of users

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

International Data Corporation (IDC) prognosticates that before the current the year gets over disbursing on IT framework products to be sent in cloud environs will be $37.1B.
The Delta outage: 650 cancelled flights, more than 1200 delayed flights, thousands of frustrated customers, tens of millions of dollars in damages – plus untold reputational damage to one of the world’s most trusted airlines. All due to a catastroph…
This video shows how to set up a shell script to accept a positional parameter when called, pass that to a SQL script, accept the output from the statement back and then manipulate it in the Shell.
This videos aims to give the viewer a basic demonstration of how a user can query current session information by using the SYS_CONTEXT function
Suggested Courses

752 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