Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

How would you refactor this?

ExpensiveQuery must only run when necessary.

DECLARE @city VARCHAR(10)
DECLARE @status INT


IF @status = 5
BEGIN
    SELECT c.Name, c.Phone
    FROM Customers c
    WHERE c.City = @city
    AND c.Status = 5
END
ELSE IF @status = 10
BEGIN
    SELECT c.Name, c.Phone
    FROM Customers c
    WHERE c.City = @city
    AND c.Status = 10
    AND c.SignUpDate NOT IN (EXEC ExpensiveQuery)
END
share|improve this question
    
The actual query is about 30 lines, 28 of which are the same most of the time. Maybe this is a time when things don't need to be dry –  user30586 Aug 26 at 18:50
1  
I think konijin's answer below is as good as this snippet is going to get. Have you considered perhaps posting ExpensiveQuery for review? Perhaps we can have more impact on your code base in general that way. –  Phrancis Aug 26 at 20:54

2 Answers 2

up vote 5 down vote accepted

I am not sure your quest for de-duplication is warranted, the only approach I know of is this:

DECLARE @city VARCHAR(10)
DECLARE @status INT

SELECT c.Name, c.Phone
FROM Customers c
WHERE c.City = @city
AND c.Status = @status
AND ( ( c.Status = 5 )
OR    ( c.Status = 10  AND c.SignUpDate NOT IN (EXEC ExpensiveQuery) ) )
share|improve this answer
    
c.status = @status is common to both branches. Is that duplication intentional? –  200_success Aug 26 at 18:07
1  
It was, because I had not considered taking it out –  konijn Aug 26 at 18:11
    
Its a contrived example, I realize now that its redundant. –  user30586 Aug 26 at 18:49
1  
This idea works but I found that ExpensiveQuery was being executed even when Status was != 10 –  user30586 Aug 26 at 18:52
2  
SQL Server evaluation order is non-deterministic, so you cannot count on short-circuiting to work. In the expression (A AND B), the query optimizer may elect to evaluate A first, B first, or both simultaneously. –  Dan Lyons Aug 26 at 23:26

You could put the duplicated part into a CTE

and then run it like this

WITH CommonTable AS 
(
    SELECT 
        c.Name
        , c.Phone
        , c.signUpDate
    FROM
        Customers c
    WHERE 
        c.City = @city 
        AND c.Status = @status
)
SELECT * FROM 
(
    IF @status = 10 
    BEGIN
        SELECT * FROM CommonTable WHERE CommonTable.SignUpDate NOT IN (EXEC ExpensiveQuery) 
    END
    ELSE
    BEGIN
        SELECT * FROM CommonTable
    END
)
share|improve this answer
    
why the down vote? does this not work? –  Malachi Aug 26 at 19:33
    
It wasnt me ;) Did you verify that SQL Server has CTE's? –  konijn Aug 26 at 19:46
    
@konijn, SQL SERVER is all I play with for SQL, but yes SQL SERVER has CTE's –  Malachi Aug 26 at 19:48
1  
Perhaps you encountered a comma-first hater ;) –  konijn Aug 26 at 19:55

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.