Optimizing sales report stored procedure

I need to optimize the following stored proc. Please let me know of any techniques or modifications that I can make to optimize this piece of code.

The procedure is for a report that needs to run and span a couple of months' worth of data.

```
ALTER PROCEDURE [dbo].[UP_Report_OverallSales_SP]
@DateType INT = 1, --default date type to recognised date
@DateFrom DATETIME = null,
@DateTo DATETIME = null ,
@BrokerEmail VARCHAR(50) = null ,
@EventName VARCHAR(50) = null,
@SellerTypeList varchar(max) = '500,550,600', --default 'Large,Indy, Platinum'
@AffiliateID INT = null,
@AffiliateCategoryID INT = NULL,
@StatusIDList varchar(max) = '2,3,5', --default 'Prcoessed,Completed, Dispatched'
@FraudStatusIDList varchar(100) = '1', --default 'green'
@LMSOrders BIT = 0
AS
BEGIN

SET NOCOUNT ON

Declare @ProcessedDateFrom DATETIME = NULL
Declare @ProcessedDateTo DATETIME = NULL
Declare @RecognisedDateFrom DATETIME = NULL
Declare @RecognisedDateTo DATETIME = NULL

IF @DateType = 1 -- 1 is recognised date
begin
set @RecognisedDateFrom = @DateFrom
set @RecognisedDateTo = @DateTo
end

IF @DateType = 2 -- 2 is processed date
begin
set @ProcessedDateFrom = @DateFrom
set @ProcessedDateTo = @DateTo
end

if @AffiliateID is null
set @AffiliateID = -1

if @AffiliateCategoryID is null
set @AffiliateCategoryID = -1

Declare @BrokerId INT = null

IF @EventName =''
SELECT @EventName = NULL

IF @BrokerEmail =''
SELECT @BrokerEmail = NULL

-- get broker id from the broker email passed as a parameter
IF @BrokerEmail IS NOT NULL
Select @BrokerId = UserId from tbUser where email = @BrokerEmail

--create seller type table
--this will have all comma separated seller type ids
Declare @SellerTypeTable Table (SellerTypeId int not null)
If @SellerTypeList is not null
Insert into @Seller

Be consistent with your naming and Capitalization of Keywords.

Some places you write null and others you write NULL, some places Declare other places DECLARE. Personally I write them all in uppercase, it's a pain, but is so much easier to read in my opinion, especially when it is in plain text and not in an IDE.

You do some weird things with variables that you don't have to do, like setting them if they weren't set, you use defaults on other input parameters but not on some of them.

if @AffiliateID is null 
    set @AffiliateID = -1

if @AffiliateCategoryID is null 
set @AffiliateCategoryID = -1

Just declare the variables with a default of -1 instead of NULL

Kind of the same thing here

IF @EventName ='' 
      SELECT @EventName = NULL

IF @BrokerEmail =''
      SELECT @BrokerEmail = NULL

-- get broker id from the broker email passed as a parameter
IF @BrokerEmail IS NOT NULL 
  Select @BrokerId = UserId from tbUser where email = @BrokerEmail

@EventName is already NULL there is no reason to set it to NULL.

@BrokerEmail is always NULL here, it starts as NULL so it skips the first if statement and then skips the next if statement because it is NULL and is not used (skimmed the rest of the code and didn't see it), what are we doing with this?

@SellerTypeList is never going to be NULL because you gave the parameter defaults so drop the if statement on this and just create the table

DECLARE @SellerTypeTable Table (SellerTypeId  INT NOT NULL)
IF @SellerTypeList IS NOT NULL 
    INSERT INTO @SellerTypeTable (SellerTypeId) SELECT ID FROM dbo.FN_SplitToINT(@SellerTypeList, ',')

Could you create a single table that can be used for other things if not used for SellerTypes?

Like with these 2 tables

--create status id table  
--this will have all comma separated status id
DECLARE @StatusIDTable TABLE (StatusID INT NOT NULL)
If @StatusIDList IS NOT NULL 
  INSERT INTO @StatusIDTable (StatusID) SELECT ID FROM dbo.FN_SplitToINT(@StatusIDList, ',')

--create fraud status id table  
--this will have all comma separated fraud status id 
DECLARE @FraudStatusIDTable TABLE (FraudStatusID INT NOT NULL)
If @FraudStatusIDList is not NULL 
  INSERT INTO @FraudStatusIDTable (FraudStatusID) SELECT ID FROM dbo.FN_SplitToINT(@FraudStatusIDList, ',')

Can you use a single Table with an extra column that specifies if they are a FraudStatus or a Status? it would be just as easy to go through the table when you add a where clause on that column.

Table names using Hungarian Notation, SIGH, probably nothing you can do about that.

Aliasing tables with one or two letters is horrible, use a full name so you know what table you are dealing with.

Do you really want to update every record like this?

UPDATE @SaleWithDatesTable SET RecognisedDate = SH2.UpdateTime
from 
(
  SELECT SH.SaleID, Min(SH.UpdateTime) as 'UpdateTime'
  from tbSaleHistory SH
      INNER JOIN @SaleWithDatesTable S ON SH.SaleID = S.SaleID
  where 
      ISNULL(SH.FraudStatusID,1) IN (SELECT fraudstatusid from @FraudStatusIDTable)
      -- SH.FraudStatusID IN (SELECT fraudstatusid from @FraudStatusIDTable) 
      and SH.statusID in (SELECT statusid from @StatusIDTable)
  GROUP BY SH.SaleID
) SH2
  INNER JOIN @SaleWithDatesTable S ON SH2.SaleID = S.SaleID

of all the places that you write comments, this should be one where you actually need a comment in the code to let people know why you are updating every row in the table.

TS.StatusID,
  TSSL.Description AS 'Status',
  tvc.venuename 'Venue',
  TCCV.CityName VenueCity,
  ISNULL(TS.FraudStatusID,1) as 'FraudStatusID',
  TTH.FaceAmount,
  TCML.currencyName as 'CurrencyName',

What table is TTH? ... wait I finally found it in this massive query it's tbTicketHistory how about TicketHistory

TSSL? how about SaleStatLookUp

Believe me, writing it the first time is a pain in the neck when you have to write them all out, but when you come back in 5 months and can tell exactly what is going on in the select with out having to search to find what the heck TCCV is supposed to be, you will be glad that you wrote out the table names.

Put some space between Select statements. That great big query is followed immediately by another small query that I almost didn't see because there wasn't any new lines between them.

And that extra query is almost useless as well, all it does is order the Temp Table that you created with the big query, Lose the Temp Table #t1 and add the ORDER BY after the WHERE clause.

That should be a good start, I am sure there is more that can be fixed here though.

I suggest making these changes, clean it up a bit, make sure it runs, and then post another question with the revised code.

StackExchange Code Review Q#61769, answer score: 4