|
| 1 | +# Engineering guidelines |
| 2 | + |
| 3 | +* [Code Reviews and Check-ins](#code-reviews-and-check-ins) |
| 4 | +* [Source Code Management](#source-code-management) |
| 5 | +* [Coding Guidelines](#coding-guidelines) |
| 6 | + |
| 7 | +## Code Reviews and Check-ins |
| 8 | + |
| 9 | +To help ensure that only the highest quality code makes its way into the project, please submit all your code changes to GitHub as PRs. This includes runtime code changes, unit test updates, and updates to samples. For example, sending a PR for just an update to a unit test might seem like a waste of time but the unit tests are just as important as the product code and as such, reviewing changes to them is also just as important. |
| 10 | + |
| 11 | +The advantages are numerous: improving code quality, more visibility on changes and their potential impact, avoiding duplication of effort, and creating general awareness of progress being made in various areas. |
| 12 | + |
| 13 | +## Source Code Management |
| 14 | + |
| 15 | +### Branch Strategy |
| 16 | + |
| 17 | +In general: |
| 18 | + |
| 19 | +* `master` has the code for the latest release on NuGet.org (e.g. alpha, beta, RC, RTM) |
| 20 | +* `dev` has the code that is being worked on but not yet released. This is the branch into which devs normally submit pull requests and merge changes into. |
| 21 | + |
| 22 | +### Solution and Project Folder Structure and Naming |
| 23 | + |
| 24 | +Solution files go in the repo root. |
| 25 | + |
| 26 | +Solutions need to contain solution folders that match the physical folders (`src`, `test`, etc.). |
| 27 | + |
| 28 | +For example, in the `Fruit` repo with the `Banana` and `Lychee` projects you would have these files checked in: |
| 29 | + |
| 30 | + /Fruit.sln |
| 31 | + /src |
| 32 | + /src/Microsoft.AspNet.Banana |
| 33 | + /src/Microsoft.AspNet.Banana/project.json |
| 34 | + /src/Microsoft.AspNet.Banana/Banana.kproj |
| 35 | + /src/Microsoft.AspNet.Banana/Banana.cs |
| 36 | + /src/Microsoft.AspNet.Banana/Util/BananaUtil.cs |
| 37 | + /src/Microsoft.AspNet.Lychee |
| 38 | + /src/Microsoft.AspNet.Lychee/project.json |
| 39 | + /src/Microsoft.AspNet.Lychee/Lychee.kproj |
| 40 | + /src/Microsoft.AspNet.Lychee/Lychee.cs |
| 41 | + /src/Microsoft.AspNet.Lychee/Util/LycheeUtil.cs |
| 42 | + /test |
| 43 | + /test/Microsoft.AspNet.Banana.Tests |
| 44 | + /test/Microsoft.AspNet.Banana.Tests/project.json |
| 45 | + /test/Microsoft.AspNet.Banana.Tests/BananaTest.kproj |
| 46 | + /test/Microsoft.AspNet.Banana.Tests/BananaTest.cs |
| 47 | + /test/Microsoft.AspNet.Banana.Tests/Util/BananaUtilTest.cs |
| 48 | + |
| 49 | +### Assembly Naming Pattern |
| 50 | + |
| 51 | +The general naming pattern is `Microsoft.Owin.Security.<area>.<subarea>`. |
| 52 | + |
| 53 | +### Build System |
| 54 | + |
| 55 | +We use [Visual Studio Team Services](https://www.visualstudio.com/en-us/products/visual-studio-team-services-vs.aspx) for builds and releases. |
| 56 | + |
| 57 | +### Samples |
| 58 | + |
| 59 | +Please ensure that all samples go in a `samples/` sub-folder in the repo. |
| 60 | + |
| 61 | +Samples should use nuget packages and not reference the source projects directly. This is so that samples can be run as standalone projects. |
| 62 | + |
| 63 | +## Coding Guidelines |
| 64 | + |
| 65 | +### Coding Style Guidelines – General |
| 66 | + |
| 67 | +The most general guideline is that we use all the VS default settings in terms of code formatting. |
| 68 | + |
| 69 | +2. Use `_camelCase` for internal and private fields and use `readonly` where possible. Prefix instance fields with `_`, static fields with `s_` and thread static fields with `t_`. When used on static fields, `readonly` should come after `static` (i.e. `static readonly` not `readonly static`). |
| 70 | +3. Avoid `this.` unless absolutely necessary |
| 71 | +4. Always specify member visiblity, even if it's the default (i.e. `private string _foo;` not `string _foo;`) |
| 72 | + |
| 73 | +### Usage of the Var Keyword |
| 74 | + |
| 75 | +The `var` keyword is to be used as much as the compiler will allow. For example, these are correct: |
| 76 | +```cs |
| 77 | +var fruit = "Banana"; |
| 78 | +var fruits = new List<Fruit>(); |
| 79 | +var flavor = fruit.GetFlavor(); |
| 80 | +string fruit = null; // can't use "var" because the type isn't known (though you could do (string)null, don't!) |
| 81 | +const string expectedName = "name"; // can't use "var" with const |
| 82 | +``` |
| 83 | + |
| 84 | +The following are incorrect: |
| 85 | +```cs |
| 86 | +string fruit = "Banana"; |
| 87 | +List<Fruit> fruits = new List<Fruit>(); |
| 88 | +FruitFlavor flavor = fruit.GetFlavor(); |
| 89 | +``` |
| 90 | + |
| 91 | +### Use C# Type Keywords in Favor of .NET Type Names |
| 92 | + |
| 93 | +When using a type that has a C# keyword the keyword is used in favor of the .NET type name. For example, these are correct: |
| 94 | + |
| 95 | +```cs |
| 96 | +public string TrimString(string s) { |
| 97 | + return string.IsNullOrEmpty(s) |
| 98 | + ? null |
| 99 | + : s.Trim(); |
| 100 | +} |
| 101 | + |
| 102 | +var intTypeName = nameof(Int32); // can't use C# type keywords with nameof |
| 103 | +``` |
| 104 | + |
| 105 | +The following are incorrect: |
| 106 | + |
| 107 | +```cs |
| 108 | +public String TrimString(String s) { |
| 109 | + return String.IsNullOrEmpty(s) |
| 110 | + ? null |
| 111 | + : s.Trim(); |
| 112 | +} |
| 113 | +``` |
| 114 | + |
| 115 | +### Line Breaks |
| 116 | + |
| 117 | +Windows uses `\r\n`, OS X and Linux uses `\n`. When it is important, use `Environment.NewLine` instead of hard-coding the line break. |
| 118 | + |
| 119 | +Note: this may not always be possible or necessary. |
| 120 | + |
| 121 | +Be aware that these line-endings may cause problems in code when using `@""` text blocks with line breaks. |
| 122 | + |
| 123 | +### File Path Separators |
| 124 | + |
| 125 | +Windows uses `\` and OS X and Linux use `/` to separate directories. Instead of hard-coding either type of slash, use [`Path.Combine()`](https://msdn.microsoft.com/en-us/library/system.io.path.combine(v=vs.110).aspx) or [`Path.DirectorySeparatorChar`](https://msdn.microsoft.com/en-us/library/system.io.path.directoryseparatorchar(v=vs.110).aspx). |
| 126 | + |
| 127 | +If this is not possible (such as in scripting), use a forward slash. Windows is more forgiving than Linux in this regard. |
| 128 | + |
| 129 | +### When to Use internals vs. public and When to Use InternalsVisibleTo |
| 130 | + |
| 131 | +As a modern set of frameworks, usage of internal types and members is allowed, but discouraged. |
| 132 | + |
| 133 | +`InternalsVisibleTo` is used only to allow a unit test to test internal types and members of its runtime assembly. We do not use `InternalsVisibleTo` between two runtime assemblies. |
| 134 | + |
| 135 | +If two runtime assemblies need to call each other's APIs, the APIs must be public. If we need it, it is likely that others need it. |
| 136 | + |
| 137 | +### Argument Null Checking |
| 138 | + |
| 139 | +To throw a runtime exception, add an explicit null check and throw an `ArgumentNullException`. Null checking is required for parameters that cannot be null (big surprise!). |
| 140 | + |
| 141 | +### Async Method Patterns |
| 142 | + |
| 143 | +By default all async methods must have the `Async` suffix. |
| 144 | + |
| 145 | +Passing cancellation tokens is done with an optional parameter with a value of `default(CancellationToken)`, which is equivalent to `CancellationToken.None` (one of the few places that we use optional parameters). |
| 146 | + |
| 147 | +Sample async method: |
| 148 | + |
| 149 | +```cs |
| 150 | +public Task GetDataAsync( |
| 151 | + QueryParams query, |
| 152 | + int maxData, |
| 153 | + CancellationToken cancellationToken = default(CancellationToken)) |
| 154 | +{ |
| 155 | + ... |
| 156 | +} |
| 157 | +``` |
| 158 | +### Use Only Complete Words or Common/Standard Abbreviations in Public APIs |
| 159 | + |
| 160 | +Public namespaces, type names, member names, and parameter names must use complete words or common/standard abbreviations. |
| 161 | + |
| 162 | +These are correct: |
| 163 | + |
| 164 | +```cs |
| 165 | +public void AddReference(AssemblyReference reference); |
| 166 | +public EcmaScriptObject SomeObject { get; } |
| 167 | +``` |
| 168 | +These are incorrect: |
| 169 | + |
| 170 | +```cs |
| 171 | +public void AddRef(AssemblyReference ref); |
| 172 | +public EcmaScriptObject SomeObj { get; } |
| 173 | +``` |
| 174 | + |
| 175 | +### Extension Method Patterns |
| 176 | + |
| 177 | +The general rule is: if a regular method would suffice, avoid extension methods. |
| 178 | + |
| 179 | +Internal extension methods are allowed, but bear in mind the previous guideline: ask yourself if an extension method is truly the most appropriate pattern. |
| 180 | + |
| 181 | +Extension methods are often difficult to mock for external developers, and thus should be avoided for public methods. |
| 182 | + |
| 183 | +The namespace of the extension method class should generally be the namespace that represents the functionality of the extension method, as opposed to the namespace of the target type. One common exception to this is that the namespace for middleware extension methods is normally always the same is the namespace of `IAppBuilder`. |
| 184 | + |
| 185 | +The class name of an extension method container (also known as a "sponsor type") should generally follow the pattern of `<Feature>Extensions`, `<Target><Feature>Extensions`, or `<Feature><Target>Extensions`. For example: |
| 186 | + |
| 187 | +```cs |
| 188 | +namespace Food |
| 189 | +{ |
| 190 | + class Fruit { ... } |
| 191 | +} |
| 192 | + |
| 193 | +namespace Fruit.Eating |
| 194 | +{ |
| 195 | + internal class FruitExtensions |
| 196 | + { |
| 197 | + public static void Eat(this Fruit fruit); |
| 198 | + } |
| 199 | + |
| 200 | + OR |
| 201 | + |
| 202 | + internal class FruitEatingExtensions |
| 203 | + { |
| 204 | + public static void Eat(this Fruit fruit); |
| 205 | + } |
| 206 | + |
| 207 | + OR |
| 208 | + |
| 209 | + internal class EatingFruitExtensions |
| 210 | + { |
| 211 | + public static void Eat(this Fruit fruit); |
| 212 | + } |
| 213 | +} |
| 214 | +``` |
| 215 | + |
| 216 | +When writing extension methods for an interface the sponsor type name must not start with an `I`. |
| 217 | + |
| 218 | +### Doc Comments |
| 219 | + |
| 220 | +The person writing the code will write the doc comments. Public APIs only. No need for doc comments on non-public types. |
| 221 | + |
| 222 | +Note: Public means callable by a customer, so it includes protected APIs. However, some public APIs might still be "for internal use only" but need to be public for technical reasons. We will still have doc comments for these APIs but they will be documented as appropriate. |
| 223 | + |
| 224 | +### Assertions |
| 225 | + |
| 226 | +Use `Debug.Assert()` to assert a condition in the code. Do not use Code Contracts (e.g. `Contract.Assert`). |
| 227 | + |
| 228 | +Please note that assertions are only for our own internal debugging purposes. They do not end up in the released code, so to alert a developer of a condition use an exception. |
| 229 | + |
| 230 | +### Tests |
| 231 | + |
| 232 | +We use the [Visual Studio unit testing framework](https://en.wikipedia.org/wiki/Visual_Studio_Unit_Testing_Framework) for all unit testing. |
| 233 | + |
| 234 | +#### Assembly Naming |
| 235 | + |
| 236 | +The tests for the `Microsoft.Fruit` assembly live in the `Microsoft.Fruit.Tests` assembly. |
| 237 | + |
| 238 | +In general there should be exactly one test assembly for each product runtime assembly. |
| 239 | + |
| 240 | +#### Test Classes and Code Coverage |
| 241 | +All test classes should be decorated with `[ExcludeFromCodeCoverage]`. |
| 242 | + |
| 243 | +#### Unit Test Class Naming |
| 244 | + |
| 245 | +Test class names end with `Tests` and live in the same namespace as the class being tested. For example, the unit tests for the `Microsoft.Fruit.Banana` class would be in a `Microsoft.Fruit.BananaTest` class in the test assembly. |
| 246 | + |
| 247 | +#### Unit Test Method Naming |
| 248 | + |
| 249 | +Unit test method names must be descriptive about _what is being tested_, _under what conditions_, and _what the expectations are_. Pascal casing should be used. The following test name is correct: |
| 250 | + |
| 251 | + PublicApiArgumentsShouldHaveNotNullAnnotation |
| 252 | + |
| 253 | +The following test names are incorrect: |
| 254 | + |
| 255 | + Test1 |
| 256 | + Constructor |
| 257 | + FormatString |
| 258 | + GetData |
| 259 | + |
| 260 | +#### Unit Test Structure |
| 261 | + |
| 262 | +The contents of every unit test should be split into three distinct stages, optionally separated by these comments: |
| 263 | + |
| 264 | +```cs |
| 265 | +// Arrange |
| 266 | +// Act |
| 267 | +// Assert |
| 268 | +``` |
| 269 | +The crucial thing here is that the `Act` stage is exactly one statement. That one statement is nothing more than a call to the one method that you are trying to test. Keeping that one statement as simple as possible is also very important. For example, this is not ideal: |
| 270 | + |
| 271 | +```cs |
| 272 | +int result = myObj.CallSomeMethod( |
| 273 | + GetComplexParam1(), |
| 274 | + GetComplexParam2(), |
| 275 | + GetComplexParam3() |
| 276 | + ); |
| 277 | +``` |
| 278 | + |
| 279 | +This style is not recommended because way too many things can go wrong in this one statement. All the `GetComplexParamN()` calls can throw for a variety of reasons unrelated to the test itself. It is thus unclear to someone running into a problem why the failure occurred. |
| 280 | + |
| 281 | +The ideal pattern is to move the complex parameter building into the `Arrange` section: |
| 282 | +```cs |
| 283 | +// Arrange |
| 284 | +P1 p1 = GetComplexParam1(); |
| 285 | +P2 p2 = GetComplexParam2(); |
| 286 | +P3 p3 = GetComplexParam3(); |
| 287 | + |
| 288 | +// Act |
| 289 | +int result = myObj.CallSomeMethod(p1, p2, p3); |
| 290 | + |
| 291 | +// Assert |
| 292 | +Assert.AreEqual(1234, result); |
| 293 | +``` |
| 294 | + |
| 295 | +Now the only reason the line with `CallSomeMethod()` can fail is if the method itself blew up. |
| 296 | + |
| 297 | +#### Use the Most Appropriate Assertion |
| 298 | + |
| 299 | +Please use the most appropriate assertion for your test. This will make the tests a lot more readable and also allow the test runner to report the best possible errors (whether it's local or the CI machine). For example, this is bad: |
| 300 | + |
| 301 | +```cs |
| 302 | +Assert.IsTrue("abc123" == someString); |
| 303 | +``` |
| 304 | + |
| 305 | +This is good: |
| 306 | + |
| 307 | +```cs |
| 308 | +Assert.AreEqual("abc123", someString); |
| 309 | +``` |
| 310 | + |
| 311 | +#### Parallel Tests |
| 312 | + |
| 313 | +By default all unit test assemblies should run in parallel mode, which is the default. Unit tests shouldn't depend on any shared state, and so should generally be runnable in parallel. If the tests fail in parallel, the first thing to do is to figure out _why_; do not just disable parallel tests! |
0 commit comments