Visual Studio Database Edition 2010 – Static Code Analysis Review (Part 2)

In Part 1 we got to see what Visual Studio Database Edition 2010 Static Code Analysis does for us. In Part 2 I’d like to take a look at what it doesn’t. Unfortunately for those who read my previous reviews of the Static Code Analysis offered by the Power Tools, it’s an all too familiar story.

Suggested Errors

CREATE
PROCEDURE [dbo].[sp_RealBadCode]

(

    @Value1 INT

    ,@Value2 INT

    — A. Unused input parameter

    ,@Value3    INT

    ,@Sum INT
OUTPUT

)

AS

— B. SET NOCOUNT ON has not been specified.

DECLARE @Comment NVARCHAR(1000)

— C. Value is defined but never used.

DECLARE @CommentChecksum INT

DECLARE @NewID INT

DECLARE @Email NVARCHAR(100)

— D. Value is defined but never used.

DECLARE @Small NVARCHAR(2)

— E. Value is assigned but never used.

SET @Email =
‘nobody@nnn.com’

— F. Reserved keyword used as table column name

SELECT [Alter]

FROM     dbo.Sums

IF
EXISTS
(SELECT 1 WHERE
LTRIM(RTRIM(@email))
LIKE
N’%[^a-z0-9._-]%@%’)

BEGIN

RAISERROR (60022, 16, 1, @Value1,‘@Email — [sp_RealBadCode]’)

— G. No use of RETURN statement.

END

— H. Resultset is not guaranteed unique, therefore the value of @Comment could be anything.

SELECT @Comment = S.Comment

FROM dbo.Sums S

WHERE S.Summation = @Sum

AND S.Value1 IN
(1,2,3)

— I. Schema / owner is not specified.

— J. No checking of @@ERROR

INSERT
INTO Sums
(Value1, Value2, Summation, Comment)

VALUES    (9, 19, 28,
‘comment 98’)

— K. Value is assigned but never used.

SET @NewID =
@@IDENTITY

Issue	Comment	Rule Suggestion
A. Unused input parameter	If a parameter is not being used in the procedure then it should not be on the interface. There may be valid exceptions to this rule, but I think it should be flagged and manually suppressed with justification.	Microsoft.Design: Review unused input parameters
B. SET NOCOUNT ON has not been specified.	I generally do this for all procedures I write. To quote Books online: ‘SET NOCOUNT ON prevents the sending of DONE_IN_PROC messages to the client for each statement in a stored procedure. For stored procedures that contain several statements that do not return much actual data, setting SET NOCOUNT to ON can provide a significant performance boost, because network traffic is greatly reduced.’.	Microsoft.Performance: Consider use of SET NOCOUNT ON
C, D. Value is defined but never used.	If you have worked with tools like Resharper, you will appreciate how handy it is for finding ‘dead wood’ code.	Microsoft.Design: Avoid unused variables.
E, K. Value is assigned but never used.	This is another Resharper type issue. It will be great for detecting unnecessary code.	Microsoft.Design: Avoid unnecessary assignment.
F. Reserved keyword used as table column name	We already have Microsoft.Naming#SR0012: Avoid reserved words for type names; however I have been unable to trigger this rule. This rule is similar and applies to schema elements, e.g. table column names. The Reserved Keyword list can be found here.	Microsoft.Naming: Avoid reserved keywords for schema elements.
G. No use of RETURN statement.	Again, it may not be the greatest example, but I can think of few reasons why not having a RETURN statement after a RAISERROR with severity > 0 should be valid.	Microsoft.Design: Consider use of RETURN statement
H. Resultset is not guaranteed unique.	Resultset is not guaranteed to return (at most) one row, therefore the value of @Comment could be any of the Comment values returned	Microsoft.Design: Resultset may be ambiguous.
I. Schema / owner is not specified..	It’s considered best practice to fully qualify objects in SQL. This can have significant performance benefits.	Microsoft.Performance: Fully qualify database objects.
J. No checking of @@ERROR.	How do we know that the INSERT won’t violate an existing (or future) constraint? The status of @@ERROR should be checked or this code should be writing using TRY-CATCH blocks.	Microsoft.Design: Check @@ERROR value after DML statement

Have you spotted any more?

Conclusion

Going back to my second review, I stated

MY V.Next Wish List

• Fix the inconsistent reporting of issues (Code Sample 2).
  
• Concise documentation at the time of release.
  
• Add the rules suggested here, and many more!
  

The first two have been done, now hopefully we can see some traction on expanding the ruleset.