Skip to content

Commit

Permalink
Merge pull request #196 from julianbartel/feature/rule-separation
Browse files Browse the repository at this point in the history 
Extract rules into separate Markdown files
  • Loading branch information
@dennisdoomen
dennisdoomen authored Nov 21, 2019
2 parents 77b199a + 6d043b2 commit cdf8dde
Show file tree
Hide file tree
Showing 125 changed files with 1,741 additions and 1,209 deletions.
82 changes: 69 additions & 13 deletions Build/default.ps1
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
$BaseDirectory = Resolve-Path ..
$ArtifactsDirectory = "$BaseDirectory\Artifacts\"
$LibDir = "$BaseDirectory\Lib"
$defaultRulePrefix = "AV"
}

task default -depends Clean, ExtractVersionsFromGit, Compile, CompileCheatsheet, BuildHtml
Expand Down Expand Up @@ -49,27 +50,78 @@ task Compile {

$outfile = "$ArtifactsDirectory\Guidelines\CSharpCodingGuidelines.md"

foreach ($file in $files) {
Write-Host "Including " $file
$content = Get-Content $file | Out-String

$content = $content.replace('%semver%', $script:Semver)
$content = $content.replace('%commitdate%', $script:CommitDate)
$content = $content.replace('![](/assets', '![](assets')
foreach ($file in $files) {
$rawContent = Get-Content $file | Out-String

$rawContent = $rawContent.replace('%semver%', $script:Semver)
$rawContent = $rawContent.replace('%commitdate%', $script:CommitDate)
$rawContent = $rawContent.replace('![](/assets', '![](assets')

# Extract the title of the section from the Frontmatter block
if ($content -match "---(.|\n)*title\: (.+)") {
$title = $Matches[2]
$title = ""
if ($rawContent -match "---(.|\n)*title\: (.+)") {
$title = $Matches[2].Trim()
}
# Extract the category from the Frontmatter block
$category = ""
if ($rawContent -match "---(.|\n)*rule_category\: (.+)") {
$category = $Matches[2].Trim()
}

# Remove the entire Frontmatter block
$content = ($content -replace '---\r?\n(.|\r?\n)+?---\r?\n', "")
$rawContent = ($rawContent -replace '---\r?\n(.|\r?\n)+?---\r?\n', "")

# Extract the category from the Frontmatter block
if ([string]::IsNullOrEmpty($category)) {
Write-Host "Including " $file
$content = $rawContent
} else {
Write-Host "Including rules of category " $category
$content = ""
foreach ($ruleFile in (Get-ChildItem "$BaseDirectory\_rules\*")) {
$rule = Get-Content $ruleFile.FullName | Out-String
if ($rule -match "---(.|\n)*rule_category\: $category") {
# Extract the title of the rule from the Frontmatter block
$ruleTitle = ""
if ($rule -match "---(.|\n)*title\: (.+)") {
$ruleTitle = $Matches[2].Trim()
}

# Extract the severity of the rule from the Frontmatter block
$ruleSeverity = ""
if ($rule -match "---(.|\n)*severity\: (.+)") {
$ruleSeverity = $Matches[2].Trim()
}

# Extract the id of the rule from the Frontmatter block
$ruleId = ""
if ($rule -match "---(.|\n)*rule_id\: (.+)") {
$ruleId = $Matches[2].Trim()
}

# Extract the id prefix of the rule from the Frontmatter block
$ruleIdPrefix = "{{ site.default_rule_prefix }}"
if ($rule -match "---(.|\n)*custom_prefix\: (.+)") {
$ruleIdPrefix = $Matches[2].Trim()
}

$content += "<h3 id=`"${ruleIdPrefix}${ruleId}`">$ruleTitle (${ruleIdPrefix}${ruleId}) <img src=`"assets/images/$ruleSeverity.png`" /></h3>`n"

# Add rule content without Frontmatter
$content += ($rule -replace '---\r?\n(.|\r?\n)+?---\r?\n', "")
$content += "`n"
}
}
}

# Replace rule prefix variable
$content = ($content -replace '{{ site.default_rule_prefix }}', $defaultRulePrefix)

# Replace cross-page relative links with local links (since everything becomes a single HTML)
$content = ($content -replace '\(\/.+?(#av\d+)\)', '($1)')
$content = ($content -replace '\(\/.+?(#\w+)\)', '($1)')

if ($title) {
$content = "<h1>$title</h1>" + $content;
$content = "<h1>$title</h1>`n" + $content;
}

Add-Content -Path $outfile $content
Expand All @@ -86,7 +138,11 @@ task CompileCheatsheet {

$outfile = "$ArtifactsDirectory\Cheatsheet\Cheatsheet.md"

(Get-Content "$BaseDirectory\_pages\Cheatsheet.md").replace('%semver%', $script:Semver).replace('%commitdate%', $script:CommitDate) | Add-Content $outfile
$content = Get-Content "$BaseDirectory\_pages\Cheatsheet.md"
$content = ($content -replace '%semver%', $script:Semver)
$content = ($content -replace '%commitdate%', $script:CommitDate)
$content = ($content -replace '{{ site.default_rule_prefix }}', $defaultRulePrefix)
Add-Content $outfile $content

Copy-Item -Path "$BaseDirectory\assets\css\CheatSheet.css" -Destination "$ArtifactsDirectory\Cheatsheet\style.css" -recurse -Force
Copy-Item -Path "$BaseDirectory\assets\Images" -Destination "$ArtifactsDirectory\Cheatsheet\Assets\Images" -recurse -Force
Expand Down
5 changes: 5 additions & 0 deletions _config.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ bing_site_verification :
yandex_site_verification :
naver_site_verification :

collections:
- rules

# Social Sharing
twitter:
username : ddoomen
Expand Down Expand Up @@ -277,3 +280,5 @@ defaults:
share: true
sidebar:
nav: "sidebar"

default_rule_prefix: "AV"
12 changes: 12 additions & 0 deletions _layouts/rule-category.html
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
layout: single
---
{% assign filtered-rules = site.rules | where: "rule_category", page.rule_category %}
{% for rule in filtered-rules %}
{% assign full_rule_id = site.default_rule_prefix | append: rule.rule_id %}
{% if rule.custom_prefix != nil %}
{% assign full_rule_id = rule.custom_prefix | append: rule.rule_id %}
{% endif %}
<h3 id="{{ full_rule_id }}">{{ rule.title }} ({{ full_rule_id }}) <img src="/assets/images/{{ rule.severity }}.png" /></h3>
{{ rule.content }}
{% endfor %}
127 changes: 3 additions & 124 deletions _pages/1000_ClassDesignGuidelines.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,127 +5,6 @@ classes: wide
search: true
sidebar:
nav: "sidebar"
---

### <a name="av1000"></a> A class or interface should have a single purpose (AV1000) ![](/assets/images/1.png)

A class or interface should have a single purpose within the system it functions in. In general, a class either represents a primitive type like an email or ISBN number, an abstraction of some business concept, a plain data structure, or is responsible for orchestrating the interaction between other classes. It is never a combination of those. This rule is widely known as the [Single Responsibility Principle](https://8thlight.com/blog/uncle-bob/2014/05/08/SingleReponsibilityPrinciple.html), one of the S.O.L.I.D. principles.

**Tip:** A class with the word `And` in it is an obvious violation of this rule.

**Tip:** Use [Design Patterns](http://en.wikipedia.org/wiki/Design_pattern_(computer_science)) to communicate the intent of a class. If you can't assign a single design pattern to a class, chances are that it is doing more than one thing.

**Note** If you create a class representing a primitive type you can greatly simplify its use by making it immutable.

### <a name="av1001"></a> Only create a constructor that returns a useful object (AV1001) ![](/assets/images/3.png)

There should be no need to set additional properties before the object can be used for whatever purpose it was designed. However, if your constructor needs more than three parameters (which violates [AV1561](/maintainability-guidelines#av1561)), your class might have too much responsibility (and violates [AV1000](#av1000)).

### <a name="av1003"></a> An interface should be small and focused (AV1003) ![](/assets/images/2.png)

Interfaces should have a name that clearly explains their purpose or role in the system. Do not combine many vaguely related members on the same interface just because they were all on the same class. Separate the members based on the responsibility of those members, so that callers only need to call or implement the interface related to a particular task. This rule is more commonly known as the [Interface Segregation Principle](https://lostechies.com/wp-content/uploads/2011/03/pablos_solid_ebook.pdf).

### <a name="av1004"></a> Use an interface rather than a base class to support multiple implementations (AV1004) ![](/assets/images/3.png)

If you want to expose an extension point from your class, expose it as an interface rather than as a base class. You don't want to force users of that extension point to derive their implementations from a base class that might have an undesired behavior. However, for their convenience you may implement a(n abstract) default implementation that can serve as a starting point.

### <a name="av1005"></a> Use an interface to decouple classes from each other (AV1005) ![](/assets/images/2.png)

Interfaces are a very effective mechanism for decoupling classes from each other:

- They can prevent bidirectional associations;
- They simplify the replacement of one implementation with another;
- They allow the replacement of an expensive external service or resource with a temporary stub for use in a non-production environment.
- They allow the replacement of the actual implementation with a dummy implementation or a fake object in a unit test;
- Using a dependency injection framework you can centralize the choice of which class is used whenever a specific interface is requested.

### <a name="av1008"></a> Avoid static classes (AV1008) ![](/assets/images/3.png)

With the exception of extension method containers, static classes very often lead to badly designed code. They are also very difficult, if not impossible, to test in isolation, unless you're willing to use some very hacky tools.

**Note:** If you really need that static class, mark it as static so that the compiler can prevent instance members and instantiating your class. This relieves you of creating an explicit private constructor.

### <a name="av1010"></a> Don't suppress compiler warnings using the `new` keyword (AV1010) ![](/assets/images/1.png)

Compiler warning [CS0114](https://docs.microsoft.com/en-us/dotnet/csharp/misc/cs0114) is issued when breaking [Polymorphism](http://en.wikipedia.org/wiki/Polymorphism_in_object-oriented_programming), one of the most essential object-orientation principles.
The warning goes away when you add the `new` keyword, but it keeps sub-classes difficult to understand. Consider the following two classes:

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

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

This will cause behavior that you would not normally expect from class hierarchies:

PocketBook pocketBook = new PocketBook();

pocketBook.Print(); // Outputs "Printing PocketBook "

((Book)pocketBook).Print(); // Outputs "Printing Book"

It should not make a difference whether you call `Print()` through a reference to the base class or through the derived class.

### <a name="av1011"></a> It should be possible to treat a derived object as if it were a base class object (AV1011) ![](/assets/images/2.png)

In other words, you should be able to use a reference to an object of a derived class wherever a reference to its base class object is used without knowing the specific derived class. A very notorious example of a violation of this rule is throwing a `NotImplementedException` when overriding some of the base-class methods. A less subtle example is not honoring the behavior expected by the base class.

**Note:** This rule is also known as the Liskov Substitution Principle, one of the [S.O.L.I.D.](http://www.lostechies.com/blogs/chad_myers/archive/2008/03/07/pablo-s-topic-of-the-month-march-solid-principles.aspx) principles.

### <a name="av1013"></a> Don't refer to derived classes from the base class (AV1013) ![](/assets/images/1.png)

Having dependencies from a base class to its sub-classes goes against proper object-oriented design and might prevent other developers from adding new derived classes.

### <a name="av1014"></a> Avoid exposing the other objects an object depends on (AV1014) ![](/assets/images/2.png)

If you find yourself writing code like this then you might be violating the [Law of Demeter](http://en.wikipedia.org/wiki/Law_of_Demeter).

someObject.SomeProperty.GetChild().Foo()

An object should not expose any other classes it depends on because callers may misuse that exposed property or method to access the object behind it. By doing so, you allow calling code to become coupled to the class you are using, and thereby limiting the chance that you can easily replace it in a future stage.

**Note:** Using a class that is designed using the [Fluent Interface](http://en.wikipedia.org/wiki/Fluent_interface) pattern seems to violate this rule, but it is simply returning itself so that method chaining is allowed.

**Exception:** Inversion of Control or Dependency Injection frameworks often require you to expose a dependency as a public property. As long as this property is not used for anything other than dependency injection I would not consider it a violation.

### <a name="av1020"></a> Avoid bidirectional dependencies (AV1020) ![](/assets/images/1.png)

This means that two classes know about each other's public members or rely on each other's internal behavior. Refactoring or replacing one of those classes requires changes on both parties and may involve a lot of unexpected work. The most obvious way of breaking that dependency is to introduce an interface for one of the classes and using Dependency Injection.

**Exception:** Domain models such as defined in [Domain-Driven Design](http://domaindrivendesign.org/) tend to occasionally involve bidirectional associations that model real-life associations. In those cases, make sure they are really necessary, and if they are, keep them in.

### <a name="av1025"></a> Classes should have state and behavior (AV1025) ![](/assets/images/1.png)

In general, if you find a lot of data-only classes in your code base, you probably also have a few (static) classes with a lot of behavior (see [AV1008](#av1008)). Use the principles of object-orientation explained in this section and move the logic close to the data it applies to.

**Exception:** The only exceptions to this rule are classes that are used to transfer data over a communication channel, also called [Data Transfer Objects](http://martinfowler.com/eaaCatalog/dataTransferObject.html), or a class that wraps several parameters of a method.

### <a name="av1026"></a> Classes should protect the consistency of their internal state (AV1026) ![](/assets/images/1.png)

Validate incoming arguments from public members. For example:

public void SetAge(int years)
{
AssertValueIsInRange(years, 0, 200, nameof(years));
this.age = years;
}

Protect invariants on internal state. For example:

public void Render()
{
AssertNotDisposed();
// ...
}
rule_category: class-design
layout: rule-category
---
55 changes: 3 additions & 52 deletions _pages/1100_MemberDesignGuidelines.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,55 +5,6 @@ classes: wide
search: true
sidebar:
nav: "sidebar"
---

### <a name="av1100"></a> Allow properties to be set in any order (AV1100) ![](/assets/images/1.png)

Properties should be stateless with respect to other properties, i.e. there should not be a difference between first setting property `DataSource` and then `DataMember` or vice-versa.

### <a name="av1105"></a> Use a method instead of a property (AV1105) ![](/assets/images/3.png)

- If the work is more expensive than setting a field value.
- If it represents a conversion such as the `Object.ToString` method.
- If it returns a different result each time it is called, even if the arguments didn't change. For example, the `NewGuid` method returns a different value each time it is called.
- If the operation causes a side effect such as changing some internal state not directly related to the property (which violates the [Command Query Separation](http://martinfowler.com/bliki/CommandQuerySeparation.html) principle).

**Exception:** Populating an internal cache or implementing [lazy-loading](http://www.martinfowler.com/eaaCatalog/lazyLoad.html) is a good exception.

### <a name="av1110"></a> Don't use mutually exclusive properties (AV1110) ![](/assets/images/1.png)

Having properties that cannot be used at the same time typically signals a type that represents two conflicting concepts. Even though those concepts may share some of their behavior and states, they obviously have different rules that do not cooperate.

This violation is often seen in domain models and introduces all kinds of conditional logic related to those conflicting rules, causing a ripple effect that significantly increases the maintenance burden.

### <a name="av1115"></a> A property, method or local function should do only one thing (AV1115) ![](/assets/images/1.png)

Similarly to rule [AV1000](/class-design-guidelines#av1000), a method body should have a single responsibility.

### <a name="av1125"></a> Don't expose stateful objects through static members (AV1125) ![](/assets/images/2.png)

A stateful object is an object that contains many properties and lots of behavior behind it. If you expose such an object through a static property or method of some other object, it will be very difficult to refactor or unit test a class that relies on such a stateful object. In general, introducing a construct like that is a great example of violating many of the guidelines of this chapter.

A classic example of this is the `HttpContext.Current` property, part of ASP.NET. Many see the `HttpContext` class as a source of a lot of ugly code. In fact, the testing guideline [Isolate the Ugly Stuff](http://codebetter.com/jeremymiller/2005/10/21/haacked-on-tdd-and-jeremys-first-rule-of-tdd/) often refers to this class.

### <a name="av1130"></a> Return an `IEnumerable<T>` or `ICollection<T>` instead of a concrete collection class (AV1130) ![](/assets/images/2.png)

You generally don't want callers to be able to change an internal collection, so don't return arrays, lists or other collection classes directly. Instead, return an `IEnumerable<T>`, or, if the caller must be able to determine the count, an `ICollection<T>`.

**Note:** If you're using .NET 4.5 or higher, you can also use `IReadOnlyCollection<T>`, `IReadOnlyList<T>` or `IReadOnlyDictionary<TKey, TValue>`.

**Exception:** Immutable collections such as `ImmutableArray<T>`, `ImmutableList<T>` and `ImmutableDictionary<TKey, TValue>` prevent modifications from the outside and are thus allowed.

### <a name="av1135"></a> Properties, arguments and return values representing strings, collections or tasks should never be `null` (AV1135) ![](/assets/images/1.png)

Returning `null` can be unexpected by the caller. Always return an empty collection or an empty string instead of a `null` reference. When your member returns `Task` or `Task<T>`, return `Task.CompletedTask` or `Task.FromResult()`. This also prevents cluttering your code base with additional checks for `null`, or even worse, `string.IsNullOrEmpty()`.

### <a name="av1137"></a> Define parameters as specific as possible (AV1137) ![](/assets/images/2.png)

If your method or local function needs a specific piece of data, define parameters as specific as that and don't take a container object instead. For instance, consider a method that needs a connection string that is exposed through a central `IConfiguration` interface. Rather than taking a dependency on the entire configuration, just define a parameter for the connection string. This not only prevents unnecessary coupling, it also improves maintainability in the long run.

**Note:** An easy trick to remember this guideline is the *Don't ship the truck if you only need a package*.

### <a name="av1140"></a> Consider using domain-specific value types rather than primitives (AV1140) ![](/assets/images/3.png)

Instead of using strings, integers and decimals for representing domain-specific types such as an ISBN number, an email address or amount of money, consider creating dedicated value objects that wrap both the data and the validation rules that apply to it. By doing this, you prevent ending up having multiple implementations of the same business rules, which both improves maintainability and prevents bugs.
rule_category: member-design
layout: rule-category
---
Loading

0 comments on commit cdf8dde

Please sign in to comment.