Blog

Code review

  • 21 January 2019
  • 0 replies
  • 98 views
Code review
Userlevel 2
Badge +1

Review required


In the Software Factory 2018.1 release, a new ‘Review required’ setting has been added to the SF configuration. This setting is turned on as default in the installation or upgrade of the Software Factory.

Review required setting

Ready for review


With review required turned on, the task to complete a control procedure will be replaced by a task to set a control procedure ready for review.


Task to set a control procedure ready for review

This task will set the status of a control procedure to a new status ‘Review’ and create a changelog entry. Like in the previous version, the changelogs, including the code changes, can be found in the ‘Code changelog’ tab page. Two new tasks have been added to code changelog.


Tasks to approve or disapprove changes

These tasks can be used by a reviewer to approve or disapprove the changes. When approved, the control procedure will be set to completed. When disapproved, a comment will be required as review feedback and the control procedure will be set back to development. The comments can be found as a detail tab page of code changelog.


Changelog comments

The developer can choose to apply the comment to the code and resolve it, or mark the comment as ‘won’t fix’ and optionally explain the decision made by replying with a comment. When done, the developer can set the procedure back to review.

A set of new control procedure icons have been added. The next table will show both new and existing icons as clarification.


Control procedure icons

Code review screen

For a reviewer, it may be useful to have a list of changelogs which are ready for review and to be able to review the code and leave comments, apart from the control procedure screen. For this purpose, a new menu item ‘Code review’ has been added to the menu group ‘Development’.


Code review screen

The functionality in this screen is basically the same as the ‘Code changelog’ screen, which was explained before. However, this screen contains prefilters to: hide approved changelogs, only show changelogs with current user as reviewer, and hide changelogs with current user as developer.


Changelog changes and comments

As can be seen above, the changes tab page shows the changes made in the code and the comments posted on the changelog.

T-SQL Code Analysis


The new Code Review screen also contains a SQL Analysis tab page, to perform a static code analysis of all T-SQL program objects using the Microsoft Data-tier Application Framework (DacFX).

After running the Code Analysis task for a specific control procedure, icons show whether there are any warnings or errors for every program object. At the moment, any issues that have to be resolved before code changes are approved are to be manually added to the code review comments. The analysis capabilities will be extended in future versions of the Software Factory.


T-SQL Code analysis

In addition to the standard Microsoft rules, rules have been added from the Microsoft DACExtensions (https://github.com/Microsoft/DACExtensions) and TSQL-Smells (https://github.com/davebally/TSQL-Smells) projects, making a total of over 60 rules.

These rules cover:
  • Type checking
  • Use of patterns in LIKE predicates
  • Potential SQL Injection Issue
  • Avoid cross server joins
  • Deprecated JOIN syntax
  • Use two part naming
  • Use of nolock / UNCOMMITTED READS
  • Use of Table / Query hints
  • Use of Select *
  • Explicit Conversion of Columnar data – Non Sargable predicates
  • Ordinal positions in ORDER BY Clauses
  • Change Of DateFormat and DateFirst
  • SET ROWCOUNT
  • Missing Column specifications on insert
  • SET OPTION usage
  • Use 2 part naming in EXECUTE statements
  • SET IDENTITY_INSERT
  • Use of RANGE windows in SQL Server 2012
  • Create table statements should specify schema
  • View created with ORDER
  • Writable cursors
  • SET NOCOUNT ON should be included inside stored procedures
  • COUNT(*) used when EXISTS/NOT EXISTS can be more performant
  • use of TOP(100) percent or TOP(>9999) in a derived table
And more…

0 replies

Be the first to reply!

Reply