Skip to main content
Commonmark migration
Source Link

###Misspellings

Misspellings

###Magic numbers

Magic numbers

###Capitalization

Capitalization

###Overall

Overall

###Misspellings

###Magic numbers

###Capitalization

###Overall

Misspellings

Magic numbers

Capitalization

Overall

Source Link
Phrancis
  • 20.5k
  • 6
  • 70
  • 155

###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!