5
\$\begingroup\$

I created a function that will return 2 dates that depend on an input parameter.

When the parameter is:

  • 1 - return start date of current week and end date of current week
  • 2 - return start date of last week and end date of last week
  • 3 - return start date of current month and end date of current month
  • 4 - return start date of last month and end date of last month

Could you please suggest improvements? And what name would be more appropriate for this function?

CREATE FUNCTION [ufn_GetRangeOfDates] 
( @Type int
)
RETURNS @Result TABLE 
( StartDate date
, EndDate date
)
AS
BEGIN
 declare @Today date = GetDate()
 -- current weak
 declare @CurrentWeakStart date = dateadd(day, 1-datepart(dw, @Today), CONVERT(date, @Today))
 declare @CurrentWeakEnd date = dateadd(day, 7-datepart(dw, @Today), CONVERT(date, @Today))
 -- last week
 declare @LastWeakStart date = DATEADD(dd, DATEPART(DW,@Today)*-1-6, @Today)
 declare @LastWeakEnd date =DATEADD(dd, DATEPART(DW,@Today)*-1, @Today)
 --current month
 declare @CurrentMonthStart date = DateAdd( day, 1 - Day( @Today ), @Today )
 declare @CurrentMonthEnd Date = DateAdd( day, -1, DateAdd( month, 1, @CurrentMonthStart ) )
 -- last month
 declare @LastMonthStart date = DATEADD(MONTH, DATEDIFF(MONTH, 0, @Today)-1, 0)
 declare @LastMonthEnd date = DATEADD(MONTH, DATEDIFF(MONTH, -1, @Today)-1, -1)
 insert into @Result
 select CASE @Type
 WHEN 1 THEN @CurrentWeakStart
 WHEN 2 THEN @LastWeakStart
 WHEN 3 THEN @CurrentMonthStart
 WHEN 4 THEN @LastMonthStart
 ELSE NULL
 END
 , CASE @Type
 WHEN 1 THEN @CurrentWeakEnd
 WHEN 2 THEN @LastWeakEnd
 WHEN 3 THEN @CurrentMonthEnd
 WHEN 4 THEN @LastMonthEnd
 ELSE NULL
 END
 RETURN
END -- End of ufn_GetRangeOfDates
asked Oct 16, 2015 at 21:21
\$\endgroup\$
3
  • 2
    \$\begingroup\$ That's a good idea! \$\endgroup\$ Commented Oct 16, 2015 at 21:31
  • \$\begingroup\$ @Phrancis Thanks a lot. What about name? \$\endgroup\$ Commented Oct 16, 2015 at 21:42
  • 1
    \$\begingroup\$ I'll try to do a review soon. I write SQL all day long so I think I might have a few ideas. \$\endgroup\$ Commented Oct 16, 2015 at 21:48

1 Answer 1

9
\$\begingroup\$

Misspellings

There are several misspelled words you should correct (see line comments):

 -- current weak -- should be 'week'
 declare @CurrentWeakStart date = dateadd(day, 1-datepart(dw, @Today), CONVERT(date, @Today)) -- should be '@CurrentWeekStart'
 declare @CurrentWeakEnd date = dateadd(day, 7-datepart(dw, @Today), CONVERT(date, @Today)) --should be '@CurrentWeekEnd'
 -- last week
 declare @LastWeakStart date = DATEADD(dd, DATEPART(DW,@Today)*-1-6, @Today) -- should be '@LastWeekStart'
 declare @LastWeakEnd date =DATEADD(dd, DATEPART(DW,@Today)*-1, @Today) -- should be '@LastWeekEnd'

Magic numbers

You use ints 1,2,3,4 as codes but no indication of what they mean. I'd suggest to take a moment to declare them and give them meaningful name:

declare @StartAndEndOfCurrentWeek int = 1;
declare @StartAndEndOfLastWeek int = 2;
declare @StartAndEndOfCurrentMonth int = 3;
declare @StartAndEndOfLastMonth int = 4;

Then this becomes more clear:

 insert into @Result
 select CASE @Type
 WHEN @StartAndEndOfCurrentWeek THEN @CurrentWeekStart
 WHEN @StartAndEndOfLastWeek THEN @LastWeekStart
 WHEN @StartAndEndOfCurrentMonth THEN @CurrentMonthStart
 WHEN @StartAndEndOfLastMonth THEN @LastMonthStart
 ELSE NULL
 END
 , CASE @Type
 WHEN @StartAndEndOfCurrentWeek THEN @CurrentWeekEnd
 WHEN @StartAndEndOfLastWeek THEN @LastWeekEnd
 WHEN @StartAndEndOfCurrentMonth THEN @CurrentMonthEnd
 WHEN @StartAndEndOfLastMonth THEN @LastMonthEnd
 ELSE NULL
 END
 RETURN

Capitalization

You should try to stick to one style of capitalization throughout for keywords. It makes it easier to read and less distracting. This is especially true of something that could have some other code mixed it, for example this one could look like there is some VBA:

DateAdd( day, 1 - Day( @Today ), @Today )

What's the difference between day and Day? Well of course there isn't in SQL, but just the fact that it's capitalized differently may throw someone off that is less used to SQL and more to programming where capitalization makes a difference.


Overall

I think in general this is a good idea and seems like a useful function to have around. Especially if you work with finance stuff a lot. I think it could reasonably be broken into two functions though, one for week and one for month. Picture something like:

  • udf_getRangeOfDatesInWeek
  • udf_getRangeOfDatesInMonth

Then you only need to supply them an int parameter and put that in your datediff() operation. 0 for current, -1 for last, 1 for next, etc. I think that would be much more flexible and useful!

answered Oct 17, 2015 at 0:45
\$\endgroup\$
1
  • 1
    \$\begingroup\$ All very good points, although I would probably keep the "magic numbers", and the reason is I would try to convert the function into an inline one and of course you can't declare variables in an inline TVF. But that doesn't matter much, because if you split the function into two, as you are suggesting at the end of your answer (which is a gold suggestion IMHO), you would avoid having magic numbers in the first place. \$\endgroup\$ Commented Oct 18, 2015 at 18:45

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.