2
\$\begingroup\$

I have code below which is set to check the date of DateToComplete, and if the date is 2 weeks or more ago, change the status of Complete from 3 to 2.

Is this the best way?

USE [DB]
GO
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO
Create PROCEDURE [dbo].[spChanger] 

AS
BEGIN

Execute ('UPDATE [TblActions] SET Complete = 2 WHERE DateToComplete < Date.Now.AddDays(14) AND Complete = 3' )

END
\$\endgroup\$
  • 1
    \$\begingroup\$ I removed the .net tag because this looks like a T-SQL CREATE PROCEDURE script, but the presence of Date.Now.AddDays(14) makes me wonder... is this working as it should? \$\endgroup\$ – Mathieu Guindon Apr 24 '14 at 15:51
  • \$\begingroup\$ If this is called from VB.NET code, I think it would be better to use an ORM (Entity Framework?) and code that logic, rather than stuffing business logic in a stored procedure. \$\endgroup\$ – Mathieu Guindon Apr 24 '14 at 16:28
5
\$\begingroup\$
Create PROCEDURE [dbo].[spChanger] 

AS
BEGIN

Execute ('UPDATE [TblActions] SET Complete = 2 WHERE DateToComplete < Date.Now.AddDays(14) AND Complete = 3' )

END

Your casing is inconsistent. If you prefer UPPERCASE keywords, stick to uppercase :)

CREATE PROCEDURE [dbo].[spChanger] 

AS
BEGIN

EXECUTE ('UPDATE [TblActions] SET Complete = 2 WHERE DateToComplete < Date.Now.AddDays(14) AND Complete = 3' )

END

The name of the procedure is potentially problematic. "Changer" says nothing about what's changing, and as your database grows you'll certainly end up wondering why you didn't call it something along the lines of spUpdateTblActionsCompleteStatusCode.


Why are you executing a string? Why not just do this?

CREATE PROCEDURE [dbo].[spChanger] 

AS
BEGIN

    UPDATE TblActions
      SET Complete = 2 
    WHERE DateToComplete < Date.Now.AddDays(14) 
      AND Complete = 3

END

Now you get IntelliSense in SSMS (assuming SQL Server) and it's much harder to make a typo on a column name.


I don't think this CREATE PROCEDURE script can run though. Date.Now.AddDays(14) isn't valid T-SQL.

That said I think you're missing opportunity for some parameters. I'd do it like this:

CREATE PROCEDURE [dbo].[spUpdateTblActionsCompleteStatusCode] 
    @completeStatusValue INT = 2,
    @daysDiff INT = 14,
    @completeStatusFilter INT = 3
AS
BEGIN

    UPDATE TblActions
      SET Complete = @completeStatusValue
    WHERE DATEDIFF(d, DateToComplete, DATEADD(d, @daysDiff, GETDATE())) < @daysDiff
      AND Complete = @completeStatusFilter

END

When your code runs this stored procedure, if no parameters are passed it will just use the default values, and you have the flexibility to pass different parameters if/when you need to.

Also I'd recommend scripting your T-SQL as a DROP+CREATE, so the full script would look like this:

USE [DB]
GO
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO

IF EXISTS (SELECT * FROM sys.objects WHERE object_id = OBJECT_ID(N'[dbo].[spUpdateTblActionsCompleteStatusCode]') AND type IN (N'P', N'PC'))
DROP PROCEDURE [dbo].[spUpdateTblActionsCompleteStatusCode]
GO

CREATE PROCEDURE [dbo].[spUpdateTblActionsCompleteStatusCode] 
    @completeStatusValue INT = 2,
    @daysDiff INT = 14,
    @completeStatusFilter INT = 3
AS
BEGIN

    UPDATE TblActions
      SET Complete = @completeStatusValue
    WHERE DATEDIFF(d, DateToComplete, DATEADD(d, @daysDiff, GETDATE())) < @daysDiff
      AND Complete = @completeStatusFilter

END
\$\endgroup\$
  • \$\begingroup\$ The reason for DROP+CREATE is that you want to be able to run the script n times, and always produce the same result; if you just CREATE, then the 2nd time you run it it will run into an error. \$\endgroup\$ – Mathieu Guindon Apr 24 '14 at 16:31
  • \$\begingroup\$ if you already have a SPROC with this name you can run an ALTER on it. rather than drop a procedure to create it again. all you have to do is change the CREATE to ALTER after you create the SPROC the first time. I think it would be a little nicer on the query. then when it is the way you want it, you run the SPROC to do the stuff inside of the SPROC \$\endgroup\$ – Malachi Apr 24 '14 at 16:33
  • \$\begingroup\$ If there are permissions to be scripted, I'd rather have them explicitly scripted alongside the CREATE part, for explicitness. \$\endgroup\$ – Mathieu Guindon Apr 24 '14 at 16:39
  • \$\begingroup\$ what is the difference between an ALTER and a CREATE the Stored Procedures that I have created and altered the only thing that changed when I needed to alter them was the Keyword? \$\endgroup\$ – Malachi Apr 24 '14 at 16:45
  • \$\begingroup\$ If there are permissions involved, ALTER wouldn't affect them; I'd prefer having the script explicitly having them, and put the .sql file under source control in the .net project. Actually it's just because I was taught to script drop+create over alter ;) \$\endgroup\$ – Mathieu Guindon Apr 24 '14 at 16:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.