Just wondering if I could solicit some feedback on a stored procedure I'm running and whether there's a more efficient way of handling the scenario (I'm pretty sure there will be!).

Basically I have a single SP that I call to return a list of records (Jobs) that may have one or more statuses and a sort order (I'm using RowNum for paging). At the moment I'm using WITH RECOMPILE because the variations on the statuses can change all the time (depending on user etc). There's also some filtering going on.

I'm using an IF statement to essentially run the same bit of code with the only change being the sort order.

I guess my questions are: Is there a better way of doing this (maybe different SP's for different statuses)? Am I overcomplicating things due to lack of knowledge (quite likely) Is the SP actually ok, but requires minor tweaks to reduce the number of lines?

I've pasted a portion of the SP below - the only difference to the full code is the additional IF statements for the different sort orders...

I'd appreciate any feedback.

Thanks in advance!

CREATE PROCEDURE [dbo].[sp_Jobs] 
    @PageNumber int, 
    @PageSize int, 
    @FilterExpression varchar(500), 
    @OrderBy varchar(50), 
    @CustomerID int, 
    @ShowNotSet bit, 
    @ShowPlaced bit, 
    @ShowProofed bit, 
    @ShowReProofed bit, 
    @ShowApproved bit, 
    @ShowOnTime bit, 
    @ShowLate bit, 
    @ShowProblem bit, 
    @ShowCompleted bit, 
    @ShowDispatched bit, 
    @ShowUnapproved bit, 
    @ShowClosed bit, 
    @ShowReturned bit, 
    @UserID int
    --JobNumber DESC 
    if @OrderBy='JobNumberDESC' 

    WITH Keys AS (SELECT TOP (@PageNumber * @PageSize) ROW_NUMBER() OVER (ORDER BY JobNumber DESC) as rn,P1.jobNumber,P1.CustID,P1.DateIn,P1.DateDue,P1.DateOut,p1.client,p1.MasterJobStatusID,p1.MasterJobStatusTimestamp,p1.OwnerID 

    vw_Jobs_List P1 WITH (NOLOCK) 

    (@CustomerID = 0 OR CustID = @CustomerID) 
    AND (@UserID = 0 OR OwnerID = @UserID) 
    AND ((@ShowNotSet = 1 AND MasterJobStatusID=1) OR (@ShowPlaced = 1 AND MasterJobStatusID=2) OR (@ShowProofed = 1 AND MasterJobStatusID=3) OR (@ShowReProofed = 1 AND MasterJobStatusID=4) OR (@ShowApproved = 1 AND MasterJobStatusID=5) OR (@ShowOnTime = 1 AND MasterJobStatusID=6) OR (@ShowLate = 1 AND MasterJobStatusID=7) OR (@ShowProblem = 1 AND MasterJobStatusID=8) OR (@ShowCompleted = 1 AND MasterJobStatusID=9) OR (@ShowDispatched = 1 AND MasterJobStatusID=10) OR (@ShowUnapproved = 1 AND MasterJobStatusID=11) OR (@ShowClosed = 1 AND MasterJobStatusID=12) OR (@ShowReturned = 1 AND MasterJobStatusID=13)) AND (Search LIKE '%'+@FilterExpression+'%')

    P1.JobNumber DESC ),SelectedKeys AS (
    SELECT TOP (@PageSize)SK.rn,SK.JobNumber,SK.CustID,SK.DateIn,SK.DateDue,SK.DateOut 

    Keys SK 

    SK.rn > ((@PageNumber-1) * @PageSize) 

    SK.JobNumber DESC) 

    SELECT SK.rn,J.JobNumber,J.OwnerID,J.Description,J.Client,SK.CustID,OrderNumber, CAST(DateAdd(d, -2, CAST(isnull(SK.DateIn,0) AS DateTime)) AS nvarchar) AS DateIn, CAST(DateAdd(d, -2, CAST(isnull(SK.DateDue,0) AS DateTime)) AS nvarchar) AS DateDue,CAST(DateAdd(d, -2, CAST(isnull(SK.DateOut,0) AS DateTime)) AS nvarchar) AS DateOut, Del_Method,Ticket#, InvoiceEmailed, InvoicePrinted, InvoiceExported, InvoiceComplete, JobStatus,j.MasterJobStatusID,j.MasterJobStatusTimestamp,js.MasterJobStatus 

    FROM SelectedKeys SK JOIN vw_Jobs_List J WITH (NOLOCK) ON j.JobNumber=SK.JobNumber JOIN tbl_SYSTEM_MasterJobStatus js WITH (NOLOCK) ON j.MasterJobStatusID=js.MasterJobStatusID 

    SK.JobNumber DESC 

--ELSE IF for other column sorting


You can use CASE statements in the ORDER BY clauses to decide which sort order to use:

  SELECT (...)
           WHEN 'JobNumberDESC' THEN sk.jobnumber
           WHEN (etc.)
         END DESC,
         CASE @OrderBy
           WHEN 'JobNumberASC' THEN sk.jobnumber
           WHEN (etc.)
         END ASC;

(You would have to split up the ascending and descending sort options like that)

One issue with that approach, though, is that in your original statement, I count four places where the ORDER BY statement occurs. That would mean repeating the long CASE statement a lot. I think we can fix that, though.

First off, sorting in a subquery usually doesn't make much sense. I don't see any reason to have keys and selectedkeys sorted, so let's remove those ORDER BY statements.

Now we're left with two ORDER BYs: the one used in the ROW_NUMBER function and the final sort. No matter what, you need to include the ORDER BY in the ROW_NUMBER function to make sure your paging works correctly, but here's the thing: now you can use that row number to sort the final result (i.e., change the final ORDER BY sk.jobnumber DESC to ORDER BY sk.rn). Now there's only one place where you need the dynamic ORDER BY clause, which is a lot better than four!

Disclaimer: I haven't tested these ideas; just brainstorming.

Another option would be to use dynamic SQL. Then you'd have something like this:

DECLARE @OrderByClause varchar(500),
        @sql           varchar(max);

SET @OrderByClause =
    CASE @OrderBy
      WHEN 'JobNumberDESC' THEN 'sk.jobnumber DESC'
      WHEN (etc)

SET @sql = 'WITH keys
    AS (SELECT TOP (@PageNumber * @PageSize)
        ROW_NUMBER() OVER (ORDER BY ' + @OrderByClause + ') AS rn' +

EXEC sp_executesql @sql, (etc.);

Just make sure you pay attention to the SQL injection threat.

| improve this answer | |
  • 1
    \$\begingroup\$ Remove WITH RECOMPILE as well. \$\endgroup\$ – JeffO Aug 31 '11 at 19:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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