3
\$\begingroup\$

I'm new to SQL and working on a monthly change calculation. I want to calculate the change between this months balance and the previous months balance.

This query is working for me but does not feel like the best syntax. Do you think this is fine, or are there any suggestions?

SELECT
 ROW_NUMBER() OVER(ORDER BY (CONVERT(varchar, effectivedate, 102)) ASC) Rn,
 CONVERT(varchar, effectivedate, 102) effectivedate,
 SUM(EuroCurrentBalance/1000000) TotalArrears,
 SUM(CASE WHEN Multiplier > 2 THEN EuroCurrentBalance/1000000 ELSE 0 END) NPLs,
 CASE WHEN ROW_NUMBER() OVER(ORDER BY (CONVERT(varchar, effectivedate, 102)) ASC) = 1 THEN 0 ELSE SUM(EuroCurrentBalance/1000000) - (LAG(SUM(EuroCurrentBalance/1000000),1,0) OVER (ORDER BY (CONVERT(varchar, effectivedate, 102)))) END MonthlyChange
 FROM
 DataWarehouse.dwa.FinanceMonthEndData
 GROUP BY
 CONVERT(varchar, effectivedate, 102)
 GO
Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Aug 28, 2014 at 12:09
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Welcome to Code Review! To make life easier for reviewers, please add sufficient context to your question. "monthly change calculation" doesn't exactly say very much to me. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. See also this meta question \$\endgroup\$ Commented Aug 28, 2014 at 12:28

1 Answer 1

5
\$\begingroup\$

The size of the horizontal scrollbar is the first smell. I'd start by breaking down the CASE WHEN's into multiple lines:

SELECT
 ROW_NUMBER() OVER(ORDER BY (CONVERT(varchar, effectivedate, 102)) ASC) Rn,
 CONVERT(varchar, effectivedate, 102) effectivedate,
 SUM(EuroCurrentBalance/1000000) TotalArrears,
 SUM(CASE WHEN Multiplier > 2 THEN EuroCurrentBalance/1000000 
 ELSE 0 
 END) NPLs,
 CASE WHEN ROW_NUMBER() OVER(ORDER BY (CONVERT(varchar, effectivedate, 102)) ASC) = 1 THEN 0 
 ELSE SUM(EuroCurrentBalance/1000000) - (LAG(SUM(EuroCurrentBalance/1000000),1,0) OVER (ORDER BY (CONVERT(varchar, effectivedate, 102)))) 
 END MonthlyChange
FROM
 DataWarehouse.dwa.FinanceMonthEndData
GROUP BY
 effectivedate
GO

Note that you can group by effectivedate without doing the conversion again.


Then I'd look at the redundant pieces: EuroCurrentBalance/1000000 is repeated quite often, and CONVERT(varchar, effectivedate, 102) as well. Consider a subquery, or a CTE:

WITH PreSelectionCTE
AS
(
 SELECT 
 CONVERT(varchar, effectivedate, 102) EffectiveDate,
 EuroCurrentBalance/1000000 Arrear,
 CASE WHEN Multiplier > 2 THEN EuroCurrentBalance/1000000
 ELSE 0
 END NPL
 FROM
 DataWarehouse.dwa.FinanceMonthEndData
)
SELECT
 ROW_NUMBER() OVER(ORDER BY EffectiveDate) Rn,
 SUM(Arrear) TotalArrears,
 SUM(NPL) NPLs,
 CASE WHEN ROW_NUMBER() OVER(ORDER BY (EffectiveDate)) = 1 THEN 0
 ELSE SUM(Arrear) - (LAG(Arrear,1,0) OVER(ORDER BY EffectiveDate))
 END MonthlyChange
FROM
 PreSelectionCTE
GROUP BY
 EffectiveDate
GO

This could probably be improved further, but at least it's much easier to read now! Note that ASC is implicit in an ORDER BY - I'd simply omit it, or specify it consistently: you have it explicit in 2 of your 3 ORDER BY's.

answered Aug 29, 2014 at 0:44
\$\endgroup\$
0

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.