SQL Server code review

SQL Stored Procedure Code Review Checklist

 

1. Stored Procedure body should be enclosed in BEGIN…END. Every Stored Procedure committed on source control should be “DROP and CREATE” and not the “ALTER”.

Eg:

IF EXISTS(SELECT 1 FROM sys.procedures WHERE name = 'USPGetPurchase')

BEGIN

DROP PROCEDURE USPGetPurchase

END

GO

CREATE PROCEDURE USPGetPurchase

 2. Stored Procedure parameters should be enclosed in parenthesis, each parameter listed on new line. Check validity of parameter data types.

 3. User-Defined Stored Procedure naming conventions to be followed as mentioned below, No Special characters to be included in the name of Stored Procedure.

Eg: USPGetPurchase

 4. Schema qualify (i.e. dbo.ClientDetails) all object references.

 5. Stored Procedure should have description and sample call(s) listed near the top.

 6. Always specify explicit column list, not a SELECT *

 7. Stored Procedure should have Error Log, Execute Statement Log and Output XML.

 8. All SQL Keywords should be in Uppercase.

 9. Is there a CASE statement without the ELSE clause? Always specify a default option even if you believe that it is impossible for that condition to happen.

 10. Use (N)VARCHAR(max) only when it is really necessary. Similar, columns/variables of short length should have a fixed size (CHAR, not VARCHAR(1), VARCHAR(2)). Also, check usage of Unicode (VARCHAR) data type, for example, usrs.uid is char(10), not a NCHAR(10), nor VARCHAR(10).

 11. Stored Procedure should do one thing and do it well. If the Stored Procedure returns multiple datasets – consider splitting into multiple Stored Procedures.

 12. Verify usage of temp tables vs. table variables (if you expect more than 200 rows to be temporarily stored – use local temp table; less than 200 – consider table variable).

 13. Stored Procedureshould not use global ## temp tables. Application procs should not be creating new “permanent” tables as an attempt to introduce “data backup/failure recovery point”.

 14. Verify usage of temp table columns – i.e. if you defining/populating columns that are not subsequently used – remove them. For example, if you define “client_id, client_name, client_number” columns, but later retrieve client_name and client_number from base tables (not temp one), then they should not be created as part of temp table.

15. Verify temp table columns data types – they should match the data type of base tables you’re populating them from. For example, if “client_name” is defined as NVARCHAR(500) in cm_client, then temp table should have it defined same way (not as VARCHAR(1000), NVARCHAR(1000), NVARCHAR(255), etc).

16. Verify temp table columns of “VARCHAR/NVARCHAR” data type – they should be created with “collate database_default” modifier to avoid errors if joins are performed on them in case-sensitive databases.

 17. Watch for data being implicitly converted between types. Implicit conversions can have unexpected results, such as truncating data or reducing performance.

 18. Is there GROUP BY <number> clause? It is deprecated, use proper GROUP BY <column> instead.

 19. Checks for record existence should be coded using IF EXISTS (…), not IF SELECT COUNT(*)>0…

 20. Verify usage of IN (…) or NOT IN (…), consider replacing with straight JOIN and/or IF EXISTS.

 21. Avoid using correlated sub-queries as part of SELECT, re-write using JOIN or CROSS APPLY. If you’re accessing same table more than once in a sub-query consider replacement with CTE instead.

 22. Consider using BEGIN TRY … END TRY … BEGIN CATCH … END CATCH for error handling; usage of

IF @@ERROR is discouraged, usage of GOTO is forbidden.

 23. Avoid using LOWER or UPPER built-in functions when trying to achieve case-insensitive compare;

 24. Verify handling of NULL values in nullable columns (or those brought in by LEFT JOIN), use ISNULL/COALESCE to ensure proper calculations or concatenation of the results. Use COALESCE() instead of ISNULL(), its faster & fail proof. Don’t use ISNUMERIC(), use TRY_PARSE() instead.

 25. Check for usage of @@IDENTITY, most likely it should be replaced with SCOPE_IDENTITY. Moreover, the OUTPUT clause is a better and safer way of capturing identity values. Don’t introduce your own “identity management” via MAX()+1.

 26. Verify usage of UNION vs. UNION ALL; if you’re bringing different record types that are already unique, use UNION ALL

 27. Verify usage of DISTINCT – are duplicate rows coming as a result of missing JOIN predicate or data quality issue?

28. Be careful with XML processing in SQL – check OPENXML vs. XML XQuery for the amount of data you’re considering to process.

 29. Verify target column list is used in INSERT statements. Be wary about INSERT…EXEC pattern to obtain data by calling another proc.

 30. Define NULL acceptance in the procedure and code accordingly.

31. If any dynamic SQL is used make sure it executes through only SP_EXECUTESQL only.

 32. Apply encryption procedures while dealing with sensitive information (Ex: Credit Card numbers, pass codes etc.).

 33. Verify and Drop the temp table at the beginning and end of the Stored Procedure.

 34. Try to use TRUNCATE instead of DELETE whenever is possible.

 35. Use CTEs instead of Sub-Queries for better code manageability and readability.

 36. Handle Divide-by-zero errors the columns/variables occurring in denominator.

 37. Always use SET NOCOUNT ON statement in the beginning of the Stored Procedure and SET NOCOUNT ON at the end of the Stored Procedure.

 38. With WHERE clause put OR conditions within brackets, otherwise they will conflict with other AND conditions and may behave differently.

 39. Always prefer using WITH (NOLOCK) for SELECT statement.

 40. Index should be created on all foreign keys.

 41. Expensive operators such as NOT LIKE should be avoided.

 42. No tables should have Insert, Update, Delete permissions.