Problem
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
Solution
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 int
s 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!