Coding GuidelineCoding Guidelines

Coding Guidelines

 

 

Skip to end of metadata

 

• Created by Jose Reyes, last modified by Jerry Haagsma on Mar 29, 2017

Go to start of metadata

 

The rules below were approved by a democratic process. Election details are at the end of each rule.

1. Coding style Do's and Don'ts

Note: Rules that contradict each other should be put in sequence with subindexes. They will become multiple choices questions when voting.

1.1 Do reach 100% "green" Resharper status / resolve ALL Resharper warnings. 

This is simpler than just pick and choose from their rules which one to follow. Note that this only means you shoould not see any errors or warnings in your file. It doesn't mean you need to apply all suggestions or hints.

There are a few additional rules that Resharper doesn't enforce but we want to.

There may be some rules (e.g. naming / capitalization rules) that sometimes match incorrectly; disable these one at a time as per 1.2 and provide justification in the comment / code review.

Approved 100% (21/21).

1.2 Do disable with comments resharper false positive.

If you believe Resharper shows a warning or error when it shoudn't, disable the rule temporarily with comment, so it shows green to other developers.

Approved 100% (18/18).

1.3 Do add a copyright comment at the top of the code file.

All code files should start with the following comment:

// Copyright © Citrix Systems, Inc.  All rights reserved.

This is a legal requirement.  It is not optional.  This applies to all files, including source files, XML files, JSON files, production code, test code, unit tests, automation, etc, etc.  There are no files excluded unless there is some technical reason why the comment cannot be included (parser can't deal with it for example).

Approved 100% (19/19).

1.4 Do add using clauses before the namespace.

So this is correct:

using  System;

 

namespace  MyNamespace {...

this is not:

namespace  MyNamespace {

     using  System;

There should not be a need to define multiple namespaces in the same source file (this is one reason to have usings after namespace, see item 1.6).

Approved 100% (21/21).

1.5 Do sort using clauses, with System namespaces first.

For instance:

using  System;

using  System.Collections.Generic;

using  Citrix.CloudServices.ServiceRuntime.ApiClient.Security;

using  Newtonsoft.Json;

Resharper can do this for you automatically.

Approved 94.4% (17/18).

1.6 Do use one source file per class, interface, enums or struct.

Each entity should be in its own file.

As a result of this, it might be useful to put related classes in a new folder (and namespace). But do not over-use subfolders.

Approved 83% (15/18).

1.7 Don't use Unity "just because" / without an explicit, current need.

Unity is overkill 99% of the time. You can create multiple constructors to explicitly declare dependencies, and everything is there in the same file:

public  EntityController() :  this (

     new  MyRequestContext())

{

}

      

public  EntityController(IMyRequestContext context) :  this (

         context,

         new  MyServiceManager(context))

{

}

 

public  EntityController(

         IMyRequestContext context,

         IMyServiceManager manager)

{

     _context = context;

     _manager = manager;

}

The only usage of Unity that is condoned is if you have written tests that rely upon it.  DO NOT use unity if you think "someday I will write a test".  Use unity ONLY if you have a test written that uses it TODAY.

Another use of unity is for dependency injection (avoid direct dependencies to constructors, so they can change without affecting your code). To do this, you could use a factory.

Approved 100% (16/16).

1.8 Do use ConfigureAwait(false) when calling async method and you understand the implications.  Do ask an experienced developer if you don't know the implications.

This is easier to use on new code. Existent code could be very complicated and some scenarios can be broken if you use this.

Approved 100% (17/17).

1.9 Don't rely on thread-local variables such as those in HttpContext.

Global & thread-local variables make code less unit testable. The correct approach is to clone the values needed in your context and then expose them through an interface.

public  interface  IMyContext

{

     string  ValueIWantToUse {  get ; }

 

}

 

public  class  MyContext : IMyContext

{

     public  MyContext()

     {

         // Access global static here

         ValueIWantToUse = ApiRequestContext.Current.TheValue

     }

 

     public  ValueIWantToUse { get ;  private  set ;}

}

 

...

public  sealed  class  MyClass

{

    public  MyClass() :  this ( new  MyContext()) ...

    public  MyClass(IMyContext context) ...

...

     var useHere = _context.ValueIWantToUse;

...

     private  IMyContext _context;

}

Approved 100% (11/11).

1.10 Don't add strings

Avoid string1 = string2 + string3. Rather, for InvariantCulture string concatenation:

1. use $"{string2}{string3}" when there is a fix number of strings to add.

2. use StringBuilder when the number of strings is unknown.

For any strings that are localizable, use the CitrixResXFileCodeGenerator to define a format string in the assembly resources, and generate a method that populates the string variables. See \eng.citrite.netftlCWCCitrixDevToolsv3.6.2

Approved 94.1% (16/17).

1.11 Do order class members.

The order should be:

{

     // Public consts

     // Constructors

     // Public properties

     // Public methods

     // Private methods

     // Private consts

     // Private static fields

     // Private read only fields

     // Private fields

}

Note that "1.11.2" was vetoed by our Architect.

Approved 93.3% (14/15).

1.13 Do use string.Empty instead of "".

Either one is ok, but we should pick one, so everybody is consistent.

Approved 100% (18/18).

1.14 Don't overuse acronyms in identifier names.

Exceptions are very well know global acronyms, like http or Api. Using Citrix acronyms that would reasonably be expected to be understood by a SME of the service is fine, but not preferred.

If you use an acronym within a source file, put a comment in the file near the first usage that defines the acronym.

Approved 100% (17/17).

1.15 Do document public classes, interfaces and their public members.

Help users of the customer, when using intellisense. Documentation is also used in help pages for Rest API service.

Approved 100% (19/19).

1.16 Do check for nulls in any public API (including constructor) parameters.

public  EntityController(

         IMyRequestContext context,

         IMyServiceManager manager)

{

     if  (context ==  null )  throw  new  ArgumentNullException(nameof(context));

     if  (manager ==  null )  throw  new  ArgumentNullException(nameof(manager));

 

     _context = context;

     _manager = manager;

}

Approved 100% (19/19).

1.17 Do make class internal unless is needed outside the assembly.

The idea is to grant the less scope possible. It's always easier to make an internal class public that the other way around.

Approved 100% (18/18).

1.18 Do make class sealed unless immediate need to subclass.

Same idea as 1.17. It's easier to allow inheritance than to revoke. If you are designing a class to be used in a compositional manner, seal it when you first declare it. A class should only be non-sealed if you are designing it with the intent that it be used as a base class for extending functionality.

Approved 93.7% (15/16).

1.19 Don't make a method internal if the class is already internal.

No need to make the method internal. The point is to reduce the change needed if the class will be promoted to public.

Approved 100% (15/15).

1.29 Do make sure new projects have a new GUID.

It's easier to forget to assign a new project GUID when clonning an existent project.

Approved 100% (14/14).

1.30 Don't store returned functions before returning then to caller.

In VS2015, the debugger will show the return value from the function that just executed in the "Auto" window.

Use this pattern:

return  await _myService.MyFunc();

Instead of:

var result = await myService.MyFunc();

return  result;

Approved 70% (7/10).

1.31 Do make sure you have the build system <Import> statement at the bottom of your project right before the Microsoft.CSharp.targets import.

You can also remove the unused commented-out XML referring to post-build actions.

Approved 100% (8/8).

1.32 Do use the nameof() keyword when referring to local parameters.

For example:

o   Instead of: if (context == null) throw new ArgumentNullException("context");

o   Use: if (context == null) throw new ArgumentNullException(nameof(context));

o   ReSharper or VS2015 will suggest this change.

Approved 91.6% (11/12).

1.33 Do use aggregation rather than subclassing for object composition.

Changing base classes in the future very frequently leads to bugs and/or limitations in terms of what can be refactored.

Approved 87.5% (7/8).

1.34 Don't add two consecutive empty lines.

One line is more than enough to add readability.

Approved 78.9% (15/19).

1.35 Don't log payloads blindly.

Be aware of everything that is being logged. Don't use object.AsString() or JsonConvert.SerializeObject(object) for models being sent to the logs. Use object.AsSafeString() or specific safe properties.

 

 1.35 Don't log payloads blindly

Choices	Your Vote	Current Result:  (22 Total Votes)
Yes		22 Votes, 100%
No		0 Votes, 0%

 

1.36 Do add the service owner to the code review.

Not only service owners have the more context on their code, but they also need to be informed of any changes happening. If something breaks, they will likely be contacted about it.Consider adding the backup owner as well.

For a full list of the service owners visit Citrix Cloud Component Owners

 

 1.36 Do add the service owner to the code review

Choices	Your Vote	Current Result:  (20 Total Votes)
Yes		19 Votes, 95%
No		1 Votes, 5%

 

 

1.37 Don't add an empty line after beginning block (character:'{') or before ending block (character:'}').

Correct:

 

public  void  Foo()

{

     var x = Bar();

     ...

}

 

 

Incorrect:

 

public  void  Foo()

{

 

     var x = Bar();

     ...

 

}

 

 

This is arbitrary, as in the case of many other rules. They are meant to try to keep the code style similar. To create a team culture.

 

 1.37 Don't add an empty line after beginning block

Choices	Your Vote	Current Result:  (21 Total Votes)
Yes		20 Votes, 95%
No		1 Votes, 4%

 

 

1.38 Don't leave empty documentation.

It's common to enter a <summary> for a method and leave the <param> and <return> tags empty. Either populate this docs with information or remove them altogether:

Correct:

 

/// <summary>

/// Registers the MVC routes

/// </summary>

/// <param name="routes">The routes collection.</param>

 

 

Incorrect:

 

/// <summary>

/// Registers the MVC routes

/// </summary>

/// <param name="routes"></param>

 

 1.38 Don't leave empty documentation

Choices	Your Vote	Current Result:  (19 Total Votes)
Yes		15 Votes, 78%
No		4 Votes, 21%

1.40 Don't hardcode the "root" customer.

Use the ContextHelper.RootCustomerName when referring to the root customer.

 

 1.40 Don't hardcode the root customer

Choices	Your Vote	Current Result:  (18 Total Votes)
Yes		16 Votes, 88%
No		2 Votes, 11%

 

1.41 Do remove all warnings from the build.

Don't summit code that is reporting warning during build. Some of the warnings makes the build fail, but some don't. So be carefull and double check before submiting to develop branch.

 

 1.41 Do remove all warnings from the build

Choices	Your Vote	Current Result:  (20 Total Votes)
Yes		19 Votes, 95%
No		1 Votes, 5%

 

1.42 Do keep the cloud service settings up to date.

If you add/remove settings from the deployment json files, remember to update the cloud service projects. Although we don't use this projects anymore for deployment, they are still used for local debugging.

 

 1.42 Do keep the cloud service settings up-to-date.

Choices	Your Vote	Current Result:  (19 Total Votes)
Yes		17 Votes, 89%
No		2 Votes, 10%

 

1.43 Do add a new line between previous member and next documentation.

Correct:

 

/// <summary>

/// Gets all public keys for the specified service.

/// </summary>

Task<IEnumerable< string >> GetPublicKeysAsync( string  serviceName, CancellationToken cancellationToken);

 

/// <summary>

/// Gets all public keys for the specified service.

/// </summary>

Task<IEnumerable< string >> GetPublicKeysAsync( string  serviceName,  string  instanceId, CancellationToken cancellationToken);

 

Incorrect:

 

/// <summary>

/// Gets all public keys for the specified service.

/// </summary>

Task<IEnumerable< string >> GetPublicKeysAsync( string  serviceName, CancellationToken cancellationToken);

/// <summary>

/// Gets all public keys for the specified service.

/// </summary>

Task<IEnumerable< string >> GetPublicKeysAsync( string  serviceName,  string  instanceId, CancellationToken cancellationToken);

 

 

 

 1,43 Do add new line between member and next documentation

Choices	Your Vote	Current Result:  (20 Total Votes)
Yes		18 Votes, 90%
No		2 Votes, 10%

 

1.44 Do avoid the use of constructor for model classes unless extra initialization is needed.

Avoid just passing public properties that can be set more descriptively using object initializer.

Correct:

 

var model =  new  NameValueModel

{

      Name =  "name1" ,

      Value =  "value1"

};

 

Incorrect:

 

var model =  new  NameValueModel( "name1" , "value1" );

 

 1.44 Do avoid the use of constructor for model classes

Choices	Your Vote	Current Result:  (19 Total Votes)
Yes		14 Votes, 73%
No		5 Votes, 26%

 

1.45 Don't create OPTIONS endpoints.

Incorrect:

 

[HttpOptions]

[CwsAuthorize(ServiceKey.NotRequired, Operation = Operation.None)]

public  HttpResponseMessage Options()

{

     ...

 

 

The OPTIONS endpoint is available globally as part of the CORS negotiations by the browser. The call must follow the protocol, which includes special headers, so it is not meant to be implemented for each controller differently.

 

 1.45 Don't create OPTIONS endpoints

Choices	Your Vote	Current Result:  (18 Total Votes)
Yes		18 Votes, 100%
No		0 Votes, 0%

 

1.46 Do name models in storage as *Entity.

This naming convention differentiate the models stored in storage from those returned to callers.

 

 1.46 Do name models in storage as Entity

Choices	Your Vote	Current Result:  (18 Total Votes)
Yes		17 Votes, 94%
No		1 Votes, 5%

 

 

2. Table Storage Do's and Don'ts

2.1 Do use small partitions.

Search is faster in small partitions.  Optimal partition size is 1 single row.

Approved 100% (11/11).

2.2 Do use prefixes for partition and row keys.

This makes data migration scenarios easier.

Approved 100% (12/12).

2.3 Do always use Partition key in searches.

Using only the row key will result in a whole-table scan which can take longer than expected when the partition is to big.

If a partition is not known outright before the search, use a partition scan, not a whole-table scan.

Approved 100% (12/12).

2.4 Do use ITableClient rather than TableUtilities.

This makes the code more unit testable.

Approved 100% (6/6).

2.5 Do define static Get Method for Partition Key and Row Key within Data Table Entity Class.

This ensures that any rules associated with the Primary / Row keys are defined only once and along with the entity definition that is using it making it easy to find as well.  Following is an example:

publicstaticstring GetPartitionKey( string  customerId,  string  provider)

{

   return  HttpUtility.UrlEncode($ "{customerId.ToLowerInvariant()}{Delimiter}{provider.ToUpperInvariant()}{Delimiter}{Location}" );

}

OR

publicstaticstring GetPartitionKey( string  customerId,  string  provider)

{

     return  HttpUtility.UrlEncode( string .Format( "{0}{1}{2}{3}{4}" ,

                         customerId.ToLowerInvariant(),

                         Delimiter,

                         provider.ToUpperInvariant(),

                         Delimiter,

                         Location));

}

 

Approved 100% (7/7).

3. Rest API Do's and Don'ts

3.1 Do return 201 Created status code for POST create operations.

If the item was actually created.

Approved 100% (13/13).

3.2 Do return 200 Ok status code for POST create operation if the item exists and didn't change.

We should not return 409 when creating an item that is already there with the exact same values. You can follow the following pattern:

var existent = GetExistentEntity(...);

if  (existent ==  null )

{

     Create(...);

     return  Created();

}

 

string  differentProperty;

if  (ObjectExtensions.ComparePublicProperties(existent, createModel,  out  differentProperty)

     return  Ok();

else

    return  Conflict($ "Property {differentProperty} is different" )

Approved 100% (13/13).

3.3 Do return 409 Conflict for POST create operation if the item is different.

This is self explanatory.

Approved 100% (14/14).

3.4 Don't pass the id of the item in the body on PUT Update.

The id should be in the URL (e.g. PUT {customer}/entity/{id})

Approved 100% (12/12).

3.5 Don't use a body payload for DELETE operations.

Only the id should be needed for a delete and it should be in the URL (e.g. DELETE {customer}/entity/{id})

Approved 100% (15/15).

3.6 Do return a boolean from DELETE operations.

Return true if the item was actually deleted. Return false if delete is in progress or if the item is not there.

Dispproved 46% (6/13). Approved by veto power of Architect TomK.

3.7 Don't return 404 Not Found for DELETE operations.

If the item is not there, them delete goal was achieved.

Approved 58.3% (6/13). Approved by veto power of Architect TomK.

3.8 Do explicitly add the route attribute in each controller action.

Explicit route is more readable and more maintainable.

[HttpPost]

[Route( "{customer}/entity" )]

[CwsAuthorize(ServiceKey.Required, Operation = Operation.None, RootOnly =  true )]

public  async Task<MyResultModel> Create([FromBody]MyCreateModel createModel)

{ ...

Approved 83.3% (10/12).

3.9 Don't throw HttpResponseException to return non-error codes.

If you need to differentiate success responses (e.g. 201 Created, 200 OK), its better to combine IHttpActionResult and ReponseTypeAttribute in your controller's action, and avoid the performance hit of exception throwing.

[ResponseType( typeof (MyResultModel))]

public  async Task<IHttpActionResult> Create([FromBody]MyCreateModel createModel)

{

     ...

     return  Created(where, result); ...

Approved 100% (10/10).

3.10 Do return 404 on GET actions but don't throw exceptions in the client library.

GET actions returning 404 should be filtered in the client library to respond with null rather than throwing a not found exception.

Approved 100% (9/9).

 

3.11 Do create thin clients.

The clients using the API should be as thin as possible. Move the majority of the logic to the API. This is true for many types of clients such as PowerShell cmdlets or connector services.

Approved 100% (10/10).

3.12 Do use different models for POST and PUT

The POST model should comprise all of the properties that the caller is allowed to specify when the object is created.

The PUT model should comprise all of the properties that the caller is allowed to change.

Very often, some properties can only be set at creation time; these should only be in the POST model.

Even if POST and PUT models are the same in version 1, they may diverge in the future, so they should be different models/classes from the start.

Approved 85.7% (12/14).

3.13 Don't return 404 on GET actions for collections.

Return an empty collection if there are no items.

Approved 100% (13/13).

3.14 Do always make the returned type a model.

Don't return simple types or collections. Exceptions are Deletes (always return bool) and GetCount actions (return int).

 

 3.14 Do always make returned type a model

Choices	Your Vote	Current Result:  (17 Total Votes)
Yes		17 Votes, 100%
No		0 Votes, 0%

 

3.14 Do always name routes in plural.

Correct:

 

[Route( "{customer}/subscriptions" )

 

Incorrect:

 

[Route( "{customer}/subscription" )

 

 3.14 Do always name routes in plural

Choices	Your Vote	Current Result:  (18 Total Votes)
Yes		16 Votes, 88%
No		2 Votes, 11%

 

3.15 Do follow Dr. Roy Thomas Fielding, PhD recommendation for REST API design or Microsoft Guidelines.

See his dissertation at http://www.ics.uci.edu/~fielding/pubs/dissertation/top.htm and Microsoft Guidelines https://github.com/Microsoft/api-guidelines/blob/master/Guidelines.md

 

 3.15 Do follow recommendations for RESTful APIs

Choices	Your Vote	Current Result:  (15 Total Votes)
Yes		12 Votes, 80%
No		3 Votes, 20%

 

4. Unit Test Do's and Don'ts

4.1 Don't use local storage for data access unit test.

Rather use a mock of the ITableClient interface.

var table = Mock.Of<ITableClient>();

Approved 100% (9/9).

5. PowerShell Do's and Don'ts

5.1 Do create PowerShell modules in C#

C# code is easier to create, debug, test and maintain that large PowerShell scripts. It the script takes more than one file, it probably needs to be in a C# module.

PowerShell modules are also better than regular console Applications. They provide better handling of input parameters and validations. Better logging and progress reporting, etc.

Approved 62.5% (5/8).

6. Logging Do's and Don'ts

6.1 Do use ILogger in services and workers.

Make sure your service and worker can be troubleshot. 

Approved 100% (13/13).

6.2 Do use ILogger.TraceMethod() extension at the beggining of public methods.

This makes logging more standard.

Approved 71.4% (5/7).

6.3 Do use ILogger.TraceMethodException() extension when logging an exception.

This makes logging more standard.

Approved 100% (7/7).

6.4 Do use ILogger Trace* extensions instead ot the Write* methods.

The latest implementation of the sumo logging have added a new SourceCode field to all sumo logs. The implementation used in Trace* methods are sligltly faster than the one in Write* methods. It's also more reliable and includes the line number where the log occurred. The Write* methods can't use this more efficient way, because Utilities library uses .Net 3.5, as is required by some users in XenDesktop team. The Traces* implementations use .Net 4.5.*.

 

 6.4 Use ILogger Trace instead of Write methods

Choices	Your Vote	Current Result:  (17 Total Votes)
Yes		16 Votes, 94%
No		1 Votes, 5%

 

7. Migration tools

7.1 Don't create migrations.

Instead, use the migration process outlined. Kes and Tom are your resources to understand the process.

As part of this rule, all current guidelines in section 7 will be discarded.

 

 7.1 Don't create migrations

Choices	Your Vote	Current Result:  (17 Total Votes)
Yes		16 Votes, 94%
No		1 Votes, 5%

 

7.1 Do add -WhatIf parameter functionality to migration tools.

Validate the possible changes before doing them for real. -WhatIf will also allow to catch bugs in your code.

Approved 100% (11/11).

7.3 Do add a lot of logs

Again, so you can validate your changes are what you expect.

Approved 100% (18/18).

7.4 Do store the logs by default in a text file.

So tool users don't have to copy to tools verbose output.

Approved 100% (7/7).

7.5 Do include filters per customer.

It's convenient to focus on a few customers first, and with when the confidence in the tool grows you can add more and more customers.

Approved 100% (7/7).

8. UI

Please see the UI Coding Guidelines