HiveBrain v1.2.0
Get Started
← Back to all entries
patternsqlMinor

Selecting the number of working days minus weekend days and UK bank holidays

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
minusnumbertheworkingweekendbankselectingandholidaysdays

Problem

I am fairly new to coding TSQL script and am looking for some second view on my script.
The goal here is to to pull in some data and amongst that data to show a field with a counting the number of working days since the order date only (i.e. minus weekend days and UK bank holidays). The SELECT script below is working as expected in that it shows results and is therefore calling the fn_WorkDays function. What would be helpful is to have a second glance of the function itself as a double check/review. The last part of the function is looking at a date field of a table that contains all of the bank holidays.

I think the script is working okay as I have checked a few going back to January, so I'm looking to get the double check to highlight any unforeseen potential errors or even code improvements.

SELECT 
cir.[PW Number] AS PWNum
--Get the correct number of working dayts since the order date by using the fn_WorkDays function
,CONVERT(VARCHAR(10),singleended2.dbo.fn_WorkDays(cir.[Install Date], GETDATE()),103) AS NumberOfDaysSinceOrderDate 
,CONVERT(VARCHAR(10), ISNULL(cir.[Install Date],'01/01/1900'),103) AS  OrderDate--Get the order dates
,ISNULL(cirRep.CurrentStage, 'Not Set') AS CurrentStage
,cir.[ID] as CircuitID
FROM Quotebase.dbo.Circuits cir
    LEFT JOIN Quotebase.dbo.CircuitReports cirRep ON Cir.[PW Number] = CirRep.PWNumber
WHERE Cir.Status='New Circuit Order' 
ORDER BY Cir.[PW Number]


Here is the function script that is called:

```
ALTER FUNCTION dbo.fn_WorkDays (@StartDate AS DATETIME, @EndDate AS DATETIME)
--Define the output data type.
RETURNS INT
AS
--Calculate the RETURN of the function.
BEGIN
RETURN (
SELECT
(DATEDIFF(dd,@StartDate, @EndDate)+1)--Start with total number of days including weekends +1 Includes the day run
-(DATEDIFF(wk,@StartDate, @EndDate)*2)--Subtact 2 days for each full weekend
-(CASE WHEN DATENAME(dw, @StartDate) = 'Sunday' --If StartDate is a Sunday, Subtract 1
THEN 1

Solution

-
ISNULL, which you use a few times, is valid T-SQL, but there is an argument for using the more portable COALESCE instead. They're not straight aliases, though, so consider whether this is appropriate.

-
Comparing the two lines

,CONVERT(VARCHAR(10),singleended2.dbo.fn_WorkDays(cir.[Install Date], GETDATE()),103) AS NumberOfDaysSinceOrderDate 
,CONVERT(VARCHAR(10), ISNULL(cir.[Install Date],'01/01/1900'),103) AS  OrderDate--Get the order dates


you assume in the second line that cir.[Install Date] might be null, but the first line passes it directly to a function which has no special-casing for that situation. Is this intentional?

-
I think you're overdoing things in

CONVERT(VARCHAR(10), ISNULL(cir.[Install Date],'01/01/1900'),103)


You're trying to convert the date to a dd/mm/yyyy string, but if it's NULL you take a string in that format, convert it with whatever the default date format happens to be (although since the day and month are the same you have good odds of getting the right result), and then convert it back. Why not push in the CONVERT and handle the fallback outside?

-
You're making assumptions about locale-related settings inside the function. It's understandable that you hard-code an assumption about which days are the weekends: you're only interested in one culture, so that's a business-level setting. But the localisation of the values returned by DATENAME is a per-connection setting. You might think that your function will always be executed in an English locale, and you might even be right, but it's a small change to replace DATENAME with DATEPART, and it makes the code slightly more robust.

(I've had experience of maintaining a system built for a British organisation but which relied implicitly on using a US locale; since my dev machine has a UK locale, lots of things broke. Don't discount the possibility that in future your code will be maintained by someone from another country who configures their dev machine differently - or even that the production machine might be moved to a server with a different locale).

-
For the benefit of future maintenance programmers, who may not be SQL-Server experts or domain experts on UK bank holidays, it would be nice to add some comments pointing out that DATEDIFF ignores @@DATEFIRST in order to be deterministic, and that a bank holiday will never be on a weekend.

Code Snippets

,CONVERT(VARCHAR(10),singleended2.dbo.fn_WorkDays(cir.[Install Date], GETDATE()),103) AS NumberOfDaysSinceOrderDate 
,CONVERT(VARCHAR(10), ISNULL(cir.[Install Date],'01/01/1900'),103) AS  OrderDate--Get the order dates
CONVERT(VARCHAR(10), ISNULL(cir.[Install Date],'01/01/1900'),103)

Context

StackExchange Code Review Q#29297, answer score: 3

Revisions (0)

No revisions yet.