1
\$\begingroup\$

The overall intent is to consolidate individual sales transactions to total debits/credits summing up the totals by year and month--retaining a running balance from start to end. This code uses multiple #temp tables. I am interested in having this code reviewed in order to find a more efficient way to achieve this.

The create table and insert statements are for the convenience of the code reviewer, should s/he choose to execute the code.

CREATE TABLE SumTest(
    POSDate datetime NULL,
    debit money NULL,
    credit money NULL,
    RollingBalance money NULL,
    expires datetime NULL,
    ExpiringDebitBalance_2013 money,
    ExpiringDebitBalance_2014 money
);

-- mimic actual sales transactions

insert into sumtest (POSDate,debit,credit,expires) values ('Jan  5 2013 12:00AM','670.00','22.00','Dec 31 2014 12:00AM');
insert into sumtest (POSDate,debit,credit,expires) values ('Jan  6 2013 12:00AM','821.00','0.00','Dec 31 2014 12:00AM');
insert into sumtest (POSDate,debit,credit,expires) values ('Mar  8 2013 12:00AM','62.00','700.00','Dec 31 2014 12:00AM');
insert into sumtest (POSDate,debit,credit,expires) values ('Mar 11 2013 12:00AM','78.00','29.00','Dec 31 2014 12:00AM');
insert into sumtest (POSDate,debit,credit,expires) values ('Mar 11 2013 12:00AM','900.00','87.00','Dec 31 2014 12:00AM');
insert into sumtest (POSDate,debit,credit,expires) values ('Apr 16 2013 12:00AM','0.00','440.00',NULL);
insert into sumtest (POSDate,debit,credit,expires) values ('Aug 18 2013 12:00AM','0.00','50.00',NULL);
insert into sumtest (POSDate,debit,credit,expires) values ('Aug 19 2013 12:00AM','470.00','200.00','Dec 31 2014 12:00AM');
insert into sumtest (POSDate,debit,credit,expires) values ('Dec 31 2012 12:00AM','1000.00','200.00','Dec 31 2013 12:00AM');
insert into sumtest (POSDate,debit,credit,expires) values ('Dec 22 2013 12:00AM','200.00','0.00','Dec 31 2014 12:00AM');
insert into sumtest (POSDate,debit,credit,expires) values ('Dec 20 2013 12:00AM','500.00','0.00','Dec 31 2014 12:00AM');
insert into sumtest (POSDate,debit,credit,expires) values ('Nov 10 2012 12:00AM','200.00','0.00','Dec 31 2013 12:00AM');
insert into sumtest (POSDate,debit,credit,expires) values ('Nov 11 2012 12:00AM','150.00','0.00','Dec 31 2013 12:00AM');
insert into sumtest (POSDate,debit,credit,expires) values ('Nov 15 2012 12:00AM','0.00','100.00',NULL);



create table #work
(   
    POSYear int NULL,
    POSMonth int NULL,
    debit money NULL,
    credit money NULL,
    RollingBalance money NULL,
    expires datetime
)

-- start with ditching the time

insert into #work
    (POSYear,POSMonth,debit,credit,expires)
select datepart(Year, POSDate) as POSYear
      ,datepart(Month, POSDate) as POSMonth
      ,debit
      ,credit
      ,expires
from sumtest



-- dump an ordered set by year,month with a rolling balance of all points/redemptions

;with distilled as
  (select POSYear,
          POSMonth,
          TotalDebit = sum(debit),
          TotalCredit = sum(credit),
          expires
   from #work
   group by POSYear,POSMonth,expires
   )

select POSYear,
       POSMonth,
       TotalDebit,
       TotalCredit,
       RollingBalance =  sum(totaldebit + (totalcredit)*-1) over ( order by POSYear, POSMonth),
       expires
into #work2
from distilled
order by POSYear,POSMonth,TotalDebit,TotalCredit



-- filter out credits lines that dont have an expiration date 

select x.POSYear,
       x.POSMonth,
       sum(x.TotalDebit) as TotalDebit,
       sum(x.TotalCredit) as TotalCredit,
       x.RollingBalance,
       EXPYear = datepart(Year, y.expires),
       EXPMonth = datepart(Month, y.expires)
into #work3
from #work2 x
    inner join #work2 y on x.posyear = y.posyear
                 and x.posmonth = y.posmonth
where y.expires is not null
group by x.posyear,x.posmonth,x.rollingbalance,y.expires
order by posyear,posmonth

-- add back in lines that are single credits, w/o debits

INSERT INTO #work3
    (POSYear,POSMonth,TotalDebit,TotalCredit,RollingBalance,EXPYear,EXPMonth)
select 
    POSYear,
    POSMonth,
    TotalDebit,
    TotalCredit,
    RollingBalance,
case 
    when expires is null then (POSYear + 1)
end as EXPYear,
case 
    when expires is null then 12
end as EXPMonth
from #work2
where (POSYear + POSMonth) NOT IN (
           select POSYear + POSMonth
            from #work3)
\$\endgroup\$
3
  • \$\begingroup\$ i can't even get this to build on SQLFiddle.com. there are a bunch of places where the column names don't match, etc please look over your post and make sure that it is correct \$\endgroup\$
    – Malachi
    Commented Nov 7, 2013 at 15:23
  • \$\begingroup\$ FIDDLE I changed some things, if you could get this to work please post the new fiddle. \$\endgroup\$
    – Malachi
    Commented Nov 7, 2013 at 16:42
  • \$\begingroup\$ If running totals is the central theme of this snippet, you might want to take a look at this detailed analysis of various calculation methods. \$\endgroup\$
    – Andriy M
    Commented Nov 19, 2013 at 4:51

2 Answers 2

2
\$\begingroup\$

You should always make sure nothing breaks if you accidentally execute your script twice.

Your script should begin with verifying whether table SumTest exists (and DROP it if it does), before creating it.

Temporary tables #work, #work2 and #work3 are never dropped. Not a biggie, but if you run the script twice you'll get an error saying there is already an object named '#work' in the database. Dropping your #temp tables is a good habit to take.

As for the naming, well, @Malachi's answer says one thing, I'll say another: be consistent. it's all that matters. - if you drag-and-drop columns and tables from the SSMS object explorer into your query, then by all means stick to UPPERCASE. Personally I find uppercase SQL is annoying to read, it's like all those keywords are jumping at you. Do what you will. I mean, chose whether you prefer UPPERCASE or lowercase, and stick to your plan.

\$\endgroup\$
2
\$\begingroup\$

it is good Coding Practice to Capitalize all of the Reserved SQL words so that it is easier to tell them apart.

words like SELECT, FROM, WHERE, WITH, INTO, GROUP BY, ORDER BY, CREATE TABLE, INSERT INTO, END, CASE, AS, IS NULL, THEN

I know this is Kind of Nitpicking, but you should get into the habit of doing it, even though SQL is not Case Sensitive on these.

I know this isn't the review that you were looking for, but it seems you have an issue that you need to fix before we can review this code

\$\endgroup\$
7
  • \$\begingroup\$ Oh boy!:-\ That's the best we can do? I thought "working code" was the operative phrase. What I have works--what I don't have is code that is good. I'm willing to admit that so I can move forward with a better approach. \$\endgroup\$
    – plditallo
    Commented Nov 1, 2013 at 14:49
  • \$\begingroup\$ I will take a closer look at it later, if I get the chance before I leave for the day, as long as the code functions. \$\endgroup\$
    – Malachi
    Commented Nov 1, 2013 at 19:25
  • \$\begingroup\$ @Malachi actually if this is written is SSMS or VisualStudio, or even NotePad++, keywords are highlighted in bright blue so IMHO they're already easy to tell apart. I like lowercase SQL, I find it's much easier on the eyes. As always what really matters is consistency: there's no reason for CREATE TABLE and NULL (vs create table and null) if you're going to insert into some values (or select from)... especially if you're later going to create table. That said I find the CR syntax highlighting could be improved. \$\endgroup\$ Commented Nov 2, 2013 at 3:01
  • \$\begingroup\$ I posted this on meta: Could syntax highlighting be improved? \$\endgroup\$ Commented Nov 2, 2013 at 4:32
  • \$\begingroup\$ I didn't necessarily mean in CR alone, I meant in the IDE, but that is a good point as well. \$\endgroup\$
    – Malachi
    Commented Nov 4, 2013 at 14:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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