- DO give PRs short-but-descriptive names (e.g. "Improve code coverage for System.Console by 10%", not "Fix #1234")
- DO refer to any related TFS Items or relevant issues.
- DO tag any users that should know about and/or review the change.
- DO ensure each commit successfully builds. The entire PR must pass all tests in the Continuous Integration (CI) system before it'll be merged.
- DO address PR feedback in an additional commit(s) rather than ammending the existing commits, and only rebase/squash them when necessary. This makes it easier for reviewers to track changes. If necessary, squashing should be handled by the merger using the "squash and merge" feature, and should only be done by the contributor upon request.
- DO NOT fix merge conflicts using a merge commit. Prefer git rebase.
- DO NOT mix independent, unrelated changes in one PR. Separate real product/test code changes from larger code formatting/dead code removal changes. Separate unrelated fixes into separate PRs, especially if they are in different assemblies.
Based on .NET Core Contributing Guide
Please format commit messages as follows (based on A Note About Git Commit Messages):
Summarize change in 50 characters or less
Provide more detail after the first line. Leave one blank line below the
summary and wrap all lines at 72 characters or less.
If the change fixes an issue, leave another blank line after the final
paragraph and indicate which issue is fixed in the specific format
below.
Fix #42
Also do your best to factor commits appropriately, not too large with unrelated things in the same commit, and not too small with the same small change applied N times in N different commits.
The general rule to follow is "use Visual Studio defaults".
- If a file happens to differ in style from these guidelines (e.g. private members are named
m_member
rather than_member
), the existing style in that file takes precedence. - DO use Allman style braces, where each brace begins on a new line. A single line statement block can go without braces but the block must be properly indented on its own line and it must not be nested in other statement blocks that use braces.
- DO use four spaces of indentation (no tabs).
- DO always specify the visibility, even if it's the default (i.e.
private string _foo
notstring _foo
). Visibility should be the first modifier (i.e.public abstract
notabstract public
). - DO specify Namespace imports at the top of the file, outside of namespace declarations and sorted alphabetically. Per StyleCop rule, System namespaces should be placed before other namespaces.
- DO NOT use more than one empty line at any time. For example, do not have two blank lines between members of a type.
- DO NOT use extra spaces. For example avoid
if (someVar == 0)...
, where the dots mark the spurious extra spaces. Consider enabling "View White Space (Ctrl+E, S)" if using Visual Studio, to aid detection. - DO NOT comment out code. It's the tracking system responsibility to keep track of the changes.
- AVOID using regions.
- DO place the members in (well-defined order)[http://stylecop.soyuz5.com/Ordering%20Rules.html].
- Constant fields / Fields
- Constructors / Destructors
- Delegates / Events
- Properties
- Methods
Within each of these groups order by access: public, internal, protected internal, protected, private
. Within each of the access groups, order by static
, then non-static
.
- DO use
_camelCase
for internal and private fields and use readonly where possible. - DO use PascalCasing for all public member, type, and namespace names consisting of multiple words. A special case is made for two-letter acronyms in which both letters are capitalized:
IOStream
. - DO use camelCasing for parameter names (
ioStream
). - DO group extension methods in a class suffixed with Extensions.
- DO post-fix asynchronous methods with
Async
orTaskAsync
. The general convention for methods that return aTask
orTask<TResult>
is to post-fix them withAsync
. - DO provide a meaningful and rich name to methods and classes. This can be tricky.
- DO NOT use abbreviations. For example, use
OnButtonClick
instead ofOnBtnClick
.
- DO use language keywords instead of BCL types (i.e.
int, string, float
instead ofInt32, String, Single
) for both type references as well as method calls (i.e.int.Parse
instead ofInt32.Parse
) - DO use
nameof(...)
instead of"..."
whenever possible and relevant. - DO use the
var
when the variable type can be implied (var name = "Lucas"
instead ofstring name = "Lucas"
andvar names = new List<string>()
instead ofList<string> names = new List<string>()
). - DO ensure that each type is a well-defined set of related members, not just a random collection of unrelated functionality.
- DO return an
ICollection<T>
orIReadOnlyCollection<T>
instead of a concrete collection class. - DO define parameters as specific as possible. If your member needs a specific piece of data, define the parameters as specific as that and don't take a container object instead.
- DO throw the most specific exception that is appropriate. For example, if a method receives a
null
argument, it should throwArgumentNullException
instead of its base typeArgumentException
. - DO use using statements instead of fully qualified type names. If you need to prevent name clashing, use a
using
directive to assign an alias. - DO prefer single (conditional) assignments instead of if-else statements.
- DO use
.Any()
instead of.Count()
when feasible. - DO implement classes or interfaces with a single responsibility. If you can't assign a single design pattern to a class, chances are that it is doing more than one thing.
- DO provide at least one type that is an implementation of an interface.
- DO use an interface instead of a base class to support multiple implementations.
- DO implement methods with a single responsibility.
- DO NOT use this. unless absolutely necessary.
- DO NOT use marker interfaces (interfaces with no members). If you need to mark a class as having a specific characteristic (marker), in general, use a custom attribute rather than an interface.
- DO NOT combine many vaguely related members on the same interface. Separate the members by responsibility, so that callers only need to call or implement the interface related to a particular task.
- DO NOT swallow errors by catching generic exception. Unless you're in a last-change exception handler, avoid catching non specific exception such as
Exception
orSystemException
. - DO NOT make explicit comparisons to true or false.
- CONSIDER defining a struct instead of a class if instances of the type are small and commonly short-lived or are commonly embedded in other objects.
Structs
should be immutable and should not be boxed frequently. - CONSIDER using object and collection initializer over separated statements.
- AVOID static classes. With the exception of extension method containers, static classes very often lead to badly designed code. They're also very difficult to test in isolation.
- AVOID using named arguments. If you need named arguments to improve the readability of the call to a method, that method is probably doing too much and should be refactored. The exception is when calling a method of some code base you don't control that has a
bool
parameter. - AVOID code duplication. If the code is used in more than one place, it should probably be extracted to a common place.
- DO apply a single space before single line comments (StyleCop:
// A single-line comment.
).
TBD.
- DO ensure it follows Pull Request standards.
- DO ensure the implementation covers the TFS acceptance criteria or fixes the issue.
- DO verify if it follows Commit Message's convention.
- DO verify if the code is covered by appropriate Unit Tests.
- DO ensure the code is compliant with security tools (such as Fortify).
- DO ensure it follows C# Coding Style guidelines.
- DO NOT get emotional.
- DO NOT focus on blame.
- DO NOT focus on how you would have coded it, instead verify if the code is maintanable/scalable and proficient.
- AVOID redesigning the code. Unless that's the specific goal of the PR, the code review is about finding nuggets of information in the code, not about revisiting design decisions.