In my opinion, this rule also applies to the ternary operator (?:).

Besides that, the existing examples have some shortcomings:

• The abbreviated names pos and val used in the examples violate AV1706 (Don't use abbreviations)
• The string null check doesn't play nicely with AV1135 (Properties, methods and arguments representing strings or collections should never be null)
• The comment // initialization should be removed (or added to both before/after examples)

Here is the standard convention that I have observed for acronyms:

• If the acronym is precisely two characters in length, use upper casing (UI IO)
• If the acronym is more than two characters in length, use Pascal casing (Http Sql)

I think this convention is consistent for public types and members throughout the foundation class library.

I noticed that developers tend to stray from this convention when it comes to variables called Id though.

I would be interested to hear your thoughts.

On my machine, the registry key HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\MSBuild\ToolsVersions\12.0 does not exist.

This causes Build.bat to fail:

    Get-ItemProperty : Cannot find path
    'HKLM:\SOFTWARE\Microsoft\MSBuild\ToolsVersions\12.0' because it does not
    exist.
    At E:\Bart\Source\Repos\CSharpGuidelines\Build\psake.psm1:625 char:61
    +         $frameworkDirs = @($buildToolsVersions | foreach { (Get-ItemProperty
    -Pa ...
    + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    ~~~
        + CategoryInfo          : ObjectNotFound: (HKLM:\SOFTWARE\...lsVersions\12
       .0:String) [Get-ItemProperty], ItemNotFoundException
        + FullyQualifiedErrorId : PathNotFound,Microsoft.PowerShell.Commands.GetIt
       emPropertyCommand

    Error: 10/18/2016 10:14:24 PM:
    At E:\Bart\Source\Repos\CSharpGuidelines\Build\psake.psm1:639 char:22 +     $fr
    ameworkDirs | foreach { Assert (test-path $_ -pathType Container) ($msgs. ... +
     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    ~~ [<<==>>] Exception: Cannot bind argument to parameter 'Path' because it is n
    ull.

Chaning "12.0" to "14.0" at line https://github.com/dennisdoomen/CSharpGuidelines/blob/master/Build/psake.psm1#L584 fixes the issue for me.

Varialbe should be variable

What is a constant variable supposed to be? Is it constant or variable, or both (never seen such thing)? :-)

Always add a block after keywords such as if, else, while, for, foreach and case (AV1535)

Be reluctant with multiple return statements (AV1540)

One entry, one exit is a sound principle and keeps control flow readable. However, if the method is very small and complies with guideline AV1500 then multiple return statements may actually improve readability over some central boolean flag that is updated at various points.

I think having multiple return statements actually improve code readability (independently of method size) when we use guard statements.

These guard statements are essential for preventing something like the Arrow Anti-Pattern.

Thoughts?

Consider adding a rule which states that types should guard their internal consistency. This is typically accomplished by:

• Argument validation from public members. For example:

public void SetAge(int years)
{
    ArgumentGuard.InRange(years, 0, 200, nameof(years));

    // ...
}

• Protecting invariants on internal state. For example:

public void Render()
{
    AssertNotDisposed();

    // ...
}

In reading up on this, I stumbled across the encapsulation principle, one of the four principles of object-oriented programming. Perhaps it's worth mentioning those in the introduction of this document, because they seem to be a foundation that many other principles are based on. And junior developers may not be aware of them.

It could be something like:

Familiarize yourself with the fundamental principles of object-oriented programming: inheritance, abstraction, encapsulation and polymorphism. [LINK]

It is easy to check just remove the 'new' keyword and you will get same result as when using it, that means that 'new' keyword does not affect at all the execution behaviour. It is intended just to make explicit your decision of hide a base class behaviour, so the compiler does not warn you any more. Usually that wont be a good implementation but C# gives you the power to decide rather than other languages like Java where members are always virtual, so we need to use it wisely; but saying that 'new' keyword hides inherited members is wrong, with 'new' or without it inherited members are hidden, the unique way to keep polymorphism is by using the 'override' keyword.

With 'new'

public class PocketBook : Book  
{
    public new void Print()
    {
        Console.WriteLine("Printing PocketBook");
    }  
}

Without 'new'

public class PocketBook : Book  
{
    public void Print()
    {
        Console.WriteLine("Printing PocketBook");
    }  
}

Then

PocketBook pocketBook = new PocketBook();
((Book)pocketBook).Print(); // Will output "Printing Book"

Both have same result:

"Printing Book"
"Printing Book"

An addition to this:

Using the 'as' is more performant when you expect that the object may be of another type. But then you must also expect the null result. If you don't expect another type, always cast with parentheses. If it is another type, a meaningful InvalidCastException will be thrown. (Which, together with try/catch, on the other side is slower than the 'as' operator.)

Mark down is broken in CheatSheet.md. Preview is showing cluttered information. Issue might be because Github MD editor is upgraded in new Github repo preview.

https://github.com/dennisdoomen/csharpguidelines/blob/master/Cheatsheet/Cheatsheet.md

File 1100_MemberDesignGuidelines.md contains what looks like a typo. It says

"Returning null can be unexpected by the caller. Always return an empty collection or an empty string instead of a null reference. This also prevents cluttering your code base with additional checks for null, or even worse, string.IsNotNullOrEmpty()."

Notice the string.IsNotNullOrEmpty(). The method is actually called "IsNullOrEmpty"

AV1535: Always add a block after keywords such as `if`, `else`, `while`, `for`, `foreach` and `case`

This rule does not mention using statements. Should we add it to the list or was it intentionally left out?

Examples:

using (var connection = GetPooledConnection())
using (var transaction = connection.BeginTransaction())
{
    ...
}

using (var connection = GetPooledConnection())
{
    using (var transaction = connection.BeginTransaction())
    {
        ...
    }
}

And maybe link to GitHub releases instead?

AV1570: Always check the result of an as operation

If you use as to obtain a certain interface reference from an object, always ensure that this operation does not return null. Failure to do so may cause a NullReferenceException at a much later stage if the object did not implement that interface. If you are sure the object is of a certain type, use the is operator instead.

The last sentence is not clear to me. If I know the target type upfront, I would use an explicit conversion (cast). The is operator today seems only useful for type checks when the derived members are not used.

Should the sentence maybe be: "If you are not sure..."?

The example in AV1220 contains a method named RaiseNotifyEvent, which does not match with the naming rule in AV1225. Consider renaming this method to OnNotifyEvent.

Nit: the method body in this example contains an extra empty line.

AV1551 mentions three overloads. The next rule (AV1553) combines them into a single method using optional parameters. But that changes behavior, because it defaults parameter count to 0. Consider changing to:

public virtual int IndexOf(string phrase, int startIndex = 0, int count = -1)
{
    var length = count == -1 ? someText.Length - startIndex : count;
    return someText.IndexOf(phrase, startIndex, length);
}

I have completed my review of all existing rules against new language features. This umbrella issue contains a checked list of rules to consider for adjustment.

Each list item here links to a separate issue to track overall progress. In the individual issues we can have some discussion. Feel free to approve, comment, propose adjustments or reject. In the latter case, a little motivation would be appreciated though. After approval, I will submit a PR to merge.

• AV1220: Use null-conditional operator in event invocations #64
• AV1739: Update for discard variables #67
• Add rule for expression-bodied members #69
• AV1200: Consider guidance on usage of throw expressions #91
• AV1500/AV1561: Consider to include local functions #92
• AV1510: Add conditions on when (not) to use using static #93
• AV1515: Include binary literals and digit separators #94
• AV1521: Scope & declaration space for out var, patterns and local functions #96
• AV1522: Multiple assignments per statement using out var, patterns and tuples #97
• AV1523: Update for dictionary initializers #98
• AV1545: Add example for null-conditional operator #102
• AV1547: Consider local functions #112
• AV1561: Update for tuples and local functions #104, #140
• AV1562: Update for out var (exclude Deconstruct) and ref returns/locals #105
• AV1570: Update for is-patterns #106
• AV1702: Add casing for tuples and local functions #107, #139
• AV1720: Include local functions #108
• AV2400: Formatting of expression bodied members and dictionary initializers #109
• AV2402: Update for ordering of using static and aliases #110
• AV2406: Add relative position of local functions #111

I had hoped to adapting these guidelines for internal use in the company I work for, while of course giving proper attribution to you.

However, the CC license chosen, 'Attribution-NonCommercial-NoDerivatives' (CC BY-NC-ND), is the most restrictive CC license available, and 'human readable' interpretations seem to agree that the license does not allow any modifications, rendering my use case in breach of license terms.

1. Creative Commons
2. Wikimedia
3. Sara F. Hawkins

I would have thought that my use case, adapting for internal use, would have been the main aim of publishing this work (which is great, BTW!), so the choice of license is puzzling. Was it your intention to restrict its use (your prerogative if so), or did you have permitting wider usage in mind?

This would enable users to click on the Rule ID in the Error List on rule violations that are reported by CSharpGuidelinesAnalyzer. Once a user clicks on the link, VS opens a browser window, similar to built-in FxCop rules.

Currently all rules within a category link to the same URL.

Applies to: "Name a source file to the type it contains (AV1506)"

Consider the next file:

public class TableLoader
{
    // ...
}

public class TableLoader<TKey, TConverter> : TableLoader
{
    // ...
}

Options:

1. Allow both types to live in the same file, because their names are the same (they only differ in arity)
2. Split into TableLoader.cs and TableLoader`2.cs (because arity = 2)
3. Split into TableLoader.cs and TableLoaderTKeyTConverter.cs
4. Split into TableLoader.cs and TableLoader.generic.cs (nested)

I think it is worth providing guidance on this situation, to align naming accross the entire team. My preference would be 2. Though I often see option 1 in codebases too.

The licensing referenced in LICENSE.md does not match the license referenced in 0001_Introduction.md

License.md -> _

Creative Commons Attribution-ShareAlike 4.0 International Public License

_

0001_Introduction.md ->

This document is published under a Creative Commons license, specifically the Attribution-NonCommercial-NoDerivatives-4.0 license.

Reported by @ggondim via CodePlex:

In most project cases, the use of "this" keyword inside a class is unregulated and some developers use it in a way an some others use in another way.

It is really important to regulate the use of the keyword. It is useful in some cases to identify a class member inside a method with many variables, but it is also useless inside a constructor, factory method or even a overloaded method.

I particullary think the use of "this" keyword is only needed on constructor calling and indexable class properties, like below:

public class MyClass
{
    public MyClass() : this(false)
    { }

    public MyClass(bool enableConfiguration)
    {
    }
}

and:

public MyClass
{
    this[int index]
    {
        get { return list.ElementAt(index); }
    }
}  

It is also a "standard" proposed in IDesign C# Coding Standard.

While I don't agree with the exception you provided (do use the type name for static method calls) for its inconsistency, I have another addition for this one:

Do use the type name as part of method names like for example ReadInt32() or GetUInt16() as in BinaryReader-like classes. These methods may be called from other CLS languages that don't know about C# type keywords. It's obvious to use a name like ReadString instead of Readstring (notice the case difference).

It's confusing to have two different code hosting sites in active use for a single project. Intro, additional links and releases are on CodePlex while the current source and issues are on GitHub. Why not pull everything together on GitHub?

I believe GitHub didn't support uploading compiled files roughly around the time of your last release. They did further back in the past, and they do again for a while. You can tag your releases in Git and attach additional files to them in the Releases section.

The readme.md file in this repository could easily hold all the info from the CodePlex start page.

Plus GitHub is more reliable and quicker than CodePlex. In fact anything is better than a Microsoft website.

Through email

Hi Dennis,

Thought that I’d drop these on email, as I’ve just reviewed your guidelines for use by our team.

The following guidelines don’t have a level (I’ve added my opinion in parenthesis).

AV1137 (2)
AV1522 (1)
AV1820 (1)
AV1825 (1)
AV1830 (1)
AV1835 (1)
AV2235 (2, as it’s related to AV1820/5)

On the cheat sheet there’s a typo in the spelling of “variable” (AV1707), and the last sentence under “Empty Lines” is strange: “Between the using statements of different companies”. I think that you mean to say “Between using statements with a different root namespace” (i.e. System.* Xunit.*)

Otherwise, they’re pretty awesome 

Cheers,
Ryan Barrett

According to the Eric Lippert's article, explicit types are used only when it's necessary:

Use explicit types if doing so is necessary for the code to be correctly understood and maintained.

I also got lots of complaints about using explicit types from the readers of the Russian translation. Visual Studio and ReSharper allow getting the information about the variable's type by just mouse covering. Whereas, var increases readability.

Could we consider this?

It seems the standard for AV1520 is far more strict than necessary and isn't supported by Eric Lippert's article that's used as a reference.

It's very rare that someone needs to know what type is used for a declaration as simple as your first "don't" example:
var i = 3;

In Eric Lippert's article, he summarizes:

• Consider using var if the code emphasizes the semantic “business purpose” of the variable and downplays the “mechanical” details of its storage.
• Use explicit types if doing so is necessary for the code to be correctly understood and maintained.

Unfortunately, it appears Uncle Bob's articles are getting harder to link to these days. The few links in the documentation to objectmentor.com now point nowhere.

https://github.com/dennisdoomen/CSharpGuidelines/search?utf8=%E2%9C%93&q=objectmentor.com

The associated icon points to ?.png, which does not exist.

Expression-bodied members were introduced in C# 6 and extended in C# 7.

Example (C# 6):

public void DisplayName() => Console.WriteLine(ToString());

Example (C# 7):

public string Name
{
    get => innerName;
    set => innerName = value;
}

The new rule could be something like: Only allow expression syntax on properties and indexers.

Is this worth mentioning / any strong feelings on this?

That doesn't quite scale. See, in the example code you need one underscore for the first parameter and already two of them for the second (because they must be unique and there is only one underscore).

Instead, I now take lambda expressions as the one and only place where I allow single-letter variable names. (Aside of maybe very short loops. Really short.) It doesn't hurt to have unused single-letter parameters here. The example code would look like this:

button.Click += (s, e) => HandleClick();

s and e are for the common sender and e. Which is wrong again because e is actually not allowed here. I mostly rename the generated 'e' parameter to args. So the two parameters should actually be called s and a. But then again, it's not very important because they're unused anyway...

Current rule:

Limit usage of fully qualified type names to prevent name clashing. For example, don't do this:

var list = new System.Collections.Generic.List();

Instead, do this:

using System.Collections.Generic;

var list = new List();

If you do need to prevent name clashing, use a using directive to assign an alias:

using Label = System.Web.UI.WebControls.Label;

Or just a verb. In OOP, methods are often operations on the instance. They are actions that the object can perform. Tell a socket to Close, or a file to Delete itself, or a collection to Clear. (See also AV1710: Don't repeat the name of a class (...) in its members) Of course the 'object' part (as in verb-object) is helpful if it's not clear from the context what thing a method will work on.

I know that PowerShell cmdlets suggest a Verb-Object naming but there's often no instance that the command applies to.

It would then be searchable in Google etc, you could CNAME csharpcodingguidelines.com to it (with Ads if need be), and then close down the codeplex site (#18).

I'm not sure how it would work if you still want to build the PDF file.

Thoughts?

You mixed those up. Point 5 really wants to say (curly) braces and point 11 means (round) parentheses. It's wrong the other way around.

Existing rule:

Declare and initialize variables as late as possible (AV1521)
Avoid the C and Visual Basic styles where all variables have to be defined at the beginning of a block, but rather define and initialize each variable at the point where it is needed.

New language features to consider:

• Out variables
  Variables can be declared in the middle of a statement, such as a method call. For example:

if (int.TryParse(text, out int result) && result > 0)
{
}

• Pattern matching
  Variables can be declared in the case clauses of a switch statement. For example:

public static double ComputeArea(object shape)
{
    switch (shape)
    {
        case Square s:
            return s.Side * s.Side;
        case Circle c:
            return c.Radius * c.Radius * Math.PI;
        case Rectangle r:
            return r.Height * r.Length;
        default:
            throw new ArgumentException(
                message: "shape is not a recognized shape",
                paramName: nameof(shape));
    }
}

• Local functions
  Can be declared at top, right-before usage and even after being used. They are not variables though, so this rule does not apply to them in the current form. Example:

public void M()
{
    L();

    void L()
    {
        Console.WriteLine("Hello");
    }
}

The link Isolate the Ugly Stuff is broken. There are several search results for this term, some of which are broken, too.

Codeplex is sun setting Dec 2017. I noticed that download link for document take you to codeplex.

Also, Please move HTML document as documentation in github so easy for user to read.

Rule title: Don't use "magic" numbers (AV1515)

Binary literals example:

byte b1 = 0b00001111;

Digit separators example:

private const int OneGigabyte = 1_073_741_824;

See https://codeblog.jonskeet.uk/2015/01/30/clean-event-handlers-invocation-with-c-6/.

Example:

Click?.Invoke(this, EventArgs.Empty);

See some potential best practices at http://gunnarpeipman.com/2017/04/charp-throw-expressions/.

Note that usage of the built-in IDE quickfix is tricky, as it may change the order of execution and break existing code.

Similar to methods, local functions should probably be constrained in their maximum number of parameters (AV1561) and maximum number of statements (AV1500). Tracked by #140.

Likewise, mentions in other rules may need to be expanded to include local functions. In most cases, it should be sufficient to replace "methods" or "members" with: "methods/members and local functions".

• A method or property should do only one thing (AV1115) #148
• Properties, methods and arguments representing strings or collections should never be null (AV1135) #149
• Define parameters as specific as possible (AV1137) #150
• Be reluctant with multiple return statements (AV1540) #151
• Encapsulate complex expressions in a method or property (AV1547) #152
• Avoid methods that take a bool flag (AV1564) #156
• Use US-English (AV1701) #157
• Use proper casing for language elements (AV1702) #115
• Don't include numbers in variables, parameters and type members (AV1704)
• Name a member, parameter or variable according to its meaning and not its type (AV1707)
• Name methods using a verb or a verb-object pair (AV1720) #108
• Post-fix asynchronous methods with Async or TaskAsync (AV1755) #153
• Consider using Any() to determine whether an IEnumerable<T> is empty (AV1800) #154
• Avoid inline comments (AV2310) #155
• Use a common layout (AV2400) #111

Additionally, we may want to provide guidance on when to (not) use local functions:

• Say something about the relative declaration location in AV2406 (Place members in a well-defined order). Options for consideration: #111
  • At the end of its containing method
  • Above its first usage
  • Below its first usage

Discussed in #96 (comment):

• Resharper suggests to convert delegate and Func variables into local functions.
• I have seen it being suggested to use for non-deferred parameter validation in yield-based deferred execution methods:

public IEnumerable<T> Enumerate<T>(IEnumerable<T> items)
{
    if (items == null)
    {
        throw new ArgumentException(nameof(items));
    }

    return InnerEnumerate(items);

    IEnumerable<T> InnerEnumerate(IEnumerable<T> array)
    {
        foreach (var item in array)
        {
            yield return item;
        }
    }
}

Do you have any explanation for this one?

This likely leads to duplicate code. You set the same default value in multiple overloads. Change one, forget the others (even though they're close to each other), and you have a problem. Your sample code would happily work if each overload only adds the next parameter and lets the other overloads add all other parameters. This way, you only add one parameter per overload and only have each value in a single place.

Obviously, if leaving out multiple parameters requires to set a number of different values, calling a longer overload is necessary. But that comes from a functional requirement.

I'd make the phrase about the order of parameters in all overloads the main point and leave out the rest.