Sunday, 26 April 2015

Code Reviews for SharePoint

Overview:  Customisation in SharePoint takes different forms and having suitable environments to test code in before setting it free in production is essential.  This post looks at various types of customization and how to code review.  As a solutions architect and when I was running the Application Development CoE for a large multinational having standards and a code review checklist help immensely.  Improving code quality and finding issues early reduces the cost of building applications so code reviews are a good idea.

There are several automated tools for performing code review that target different application platforms (think FTC in SP2010 vs App Model in SP2013).  When automating the tools, it is good to select the templates/rules that match your organisation and maturity.  Ensure you customise the rules so they not reporting issues when in fact these are your standards (an example is naming in FxCop differs from the SharePoint code naming conventions used by different businesses).

Note: The code review requires depends on CSOM, FTC or JavaScript.  Depending on what is being created/built will require different code review.

There are several automation tools that can help identify poor quality code early within the development process.  Like peer reviews, these tools can help developers implement their code in the correct manner.

Note:  Define your coding standards, have up to date architecture diagrams for architects and have the rules when and what features your developers can use.  It's fairly common for outsourcing companies to build a solution to find out you don't allow the technology they have built with.  I remember an InfoPath based solution coming into my app development center a few years ago and they could not understand that the organisation would not merely turn on InfoPath.

Note: A lot of the tools we previously used in SP 2010 for FTC solutions are not relevant to SP 2013, namely SPDisposeCheck.

Code Review Tooling Options:
  1. Visual Studio
  2. FxCop (Config in VS so it runs with the same rule set as SPCAF)
  3. StyleCop (Config in VS so it runs with the same rule set as SPCAF - forces enforcement of code style at design time)
  4. SPDisposeCheck (SP 2010 only, don't use in SP2013 even for FTC solutions)
  6. SPCAF (SharePoint Code Analysis Framework)
  7. Black Duck - Build into CI/CD pipeline checks for open source software and identifies potential security issues and highlights licencing concerns.

The 3 areas where code reviews can be performed are:
  1. Developer at run time (think Visual Studio)
  2. Continuous delivery (think gated check-ins)
  3. Formal Code reviews (think solutions architect and quality manager) 
Manually reviewing code is better than nothing (automate where possible) and some basic rules and guidance is provided below.

Code reviews improve maintainability, pick up bugs, ensure efficient code, code that shall run in production, improve security, performance and reduce the total cost of ownership.  Automated tools are worth considering and the top tool for me is SPCAF.  Do code review early, often and automate.

JavaScript Code Review Checklist:

1.> Project Structure - js into script folder in the solution file (group images, css, js and file types so the projects are easy to understand and consistent in layout)
2.> use strict directive on all pages "use strict";
3.> Always use Javascript namespaces - avoid conflicts
4.> Move hard coding to constants at the top of the file, not single use meaningful info like undefined in.  Move declarations to the top.

5.> Only used approved frameworks like jquery, notify if any other frameworks are used.
6.> Commenting.  Ensure method names tell coders what the method is performing.  Add comments that explain the method.  Don't be afraid to add value by adding inline comments. 
7.> Display friendly messages to the users if something goes wrong and add error handling to tracking /logging such as console.log() or log to ULS from an app using the provide JS api or log to a common logging mechanism.
8.> Single spacing  (no flower potting)
9.> Remove commented out code/unused comment out calls etc.
10.> Always end your switch statements with a default statement.
11.> Ensure coding standard are consistent consider using
12.> Code adheres to your agreed coding standards and example is

C# Coding Standards for SharePoint
This is a checklist, the recommendations need to be matched to your business and some scenarios such as complied C# for PowerShell plugin won’t use all the items in this checklist.
  1. Have you followed the Enterprise design guidelines, branding guidelines and coding standards.
  2. Have you used the Commenting standards e.g.
  3. Avoid declaring inline literal strings
  4. Check empty string using length e.g. if (email.Length()=0) don't use if (email.Empty || email = "")
  5. Use StringBuilder for concatenation don’t keep appending strings
  6. Return Empty array rather than null
  7. Methods must be short and focused.  Method names must be meaningful
  8. Use method Overloading, not different names for the same method.  Try keep Classes small i..e under 500 lines.  If larger use #Regions to split up the code.  Pass objects into Methods rather than multiple variables if more than 6 parameters.
  9. Enumerators should be used where possible, code is more understandable and options are easy to reuse.
  10. Only import namespaces you need and dlls.  Split code into separate assemblies and use company standard naming with appropriate namespaces naming.
  11. Make helper functions i.e. don't rewrite code several times - refactor
  12. Open connections (SQL and SharePoint) as late as possible and ensure you wrap in error handling and dispose of connections in the finally statement
  13. Reuse core code libraries (ensure commonly re-used functionality is add into core libraries cross-cutting concerns/logging/ email)
  14. Use exception Management/Try catch.  Try catch must try catch specific errors and lastly catch all errors.  No business logic must rely on using catch statements.  Don't throw exceptions within exceptions.  Catch errors as specifically as possible, die gracefully and appropriately, log the errors using the CoE code core block that puts exceptions in the farms ULS and event viewer.   And potentiall the enterprise logging platform.
  15. Dispose of SPSite and SPWeb Server site objects where appropriate. Run before deployment
  16. Run stylecop and code analysis on code regularly and before deployment
  17. Your code is x64 bit compiled. 
Have a common code/core code library that deals with cross cutting concerns, logging, caching etc.

using Microsoft.Practices.ServiceLocation;
using Microsoft.Practices.SharePoint.Common.ServiceLocation;
using Microsoft.Practices.SharePoint.Common.Logging;
ILogger _logger = SharePointServiceLocator.GetCurrent().GetInstance<ILogger>();
Exception ex = new ApplicationException("This is my test exception");
Security in C# and SP
  1. Plain text passwords are not in stored Web.config, Machine.config, or any files that contain configuration settings. 
  2. Input surfaces such as application pages, site pages, web parts and other customizations perform client and server side validation to protect from cross-site scripting (XSS) and SQL injection. 
  3. Minimal use of elevated privileges to interact with SharePoint objects. 
  4. Sensitive data is not stored in URLs, unencrypted cookies in hidden form fields, query strings or with code. 


Section 508 US Standard to ensure federal agencies 
WCAG 2.1 compliant standard should be adhered to and will cover: Jaws/Browser testing, screen zooms and brail readers.  WCAG 2.2 is due out in 2021.
RWD testing e.g. Mobile/Phone testing

SQL Standards (Establish SQL standards), a small example is:

  1. No spacing in naming objects
  2. Do not use reserves words in SQL
  3. Name tables in sigular e.g. "Patient" not "Patients"
  4. No Underscore in table naming and use Camel case e.g. "PatientResult", underscores are fine in column and Store proc naming.  
  5. Do not prefix tables e.g. "tbl_Patient" or "tblPatient" 
  6. Prefix view with "vw" e.g. vwPatientHistory
  7. Boolean columns prefixed with "Is" e.g. IsActive
  8. Stored Procs prefix with "usp" not "sp".  E.g. uspDeletePatient, use the format usp_Verb_Noun
  9. Prefix functions with ufn 
  10. label foreign key using the prefix fk and follow the format fkTableColumn e.g. fkPatientId 
  11. Make your -SQL readable not on 1 line.  Use line-breaks, no empty lines and indent spacing to make the code readable.
  12. How to comment must be standardised

This list goes on but as a starting point...  Pls post if you feel anything else is relevant.


Post a Comment