-
Notifications
You must be signed in to change notification settings - Fork 12
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
Showing
13 changed files
with
592 additions
and
232 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,19 +1,35 @@ | ||
# Rule AG0001 | ||
# Avoid direct usage of DependencyResolver | ||
|
||
<p> | ||
The <code>DependencyResolver</code> should not be used directly in the codebase. We should expose our dependencies as a constructor parameter. | ||
</p> | ||
ID: AG0001 | ||
|
||
<h2>Noncompliant Code Example</h2> | ||
<pre> | ||
var exampleService = DependencyResolver.Current.GetService<IExampleService>(); | ||
</pre> | ||
Type: Code Smell | ||
|
||
<h2>Compliant Code Example</h2> | ||
<pre> | ||
public class ExampleClass | ||
{ | ||
public ExampleClass(IExampleService exampleService) { } | ||
} | ||
Direct usage of `DependencyResolver` creates tight coupling and makes code harder to test. Dependencies should be explicitly declared through constructor injection, which promotes: | ||
|
||
</pre> | ||
- Better testability through clear dependency declaration | ||
- Improved maintainability by making dependencies visible | ||
- Stronger adherence to SOLID principles, particularly Dependency Inversion | ||
|
||
#### Don't | ||
|
||
```c# | ||
var exampleService = DependencyResolver.Current.GetService<IExampleService>(); | ||
``` | ||
|
||
#### Do | ||
|
||
```c# | ||
public class ExampleClass | ||
{ | ||
public ExampleClass(IExampleService exampleService) { } | ||
} | ||
``` | ||
|
||
The use of `DependencyResolver.Current` creates several problems: | ||
|
||
- It makes unit testing more difficult since you can't easily mock dependencies | ||
- It hides class dependencies, making the code harder to understand and maintain | ||
- It bypasses the dependency injection container's lifecycle management | ||
- It creates a direct dependency on the DI container, violating the Service Locator anti-pattern | ||
|
||
Always prefer constructor injection to make dependencies explicit and improve code quality. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,38 +1,56 @@ | ||
# Rule AG0013 | ||
|
||
<p> | ||
Limit number of test method input parameters to 5 | ||
</p> | ||
<p> | ||
Overuse of [TestCase] can make it difficult to see what is actually being tested, especially when there are many parameters. | ||
This can results in a combinatorial explosion of cases as new parameters are added. | ||
</p> | ||
|
||
<h2>Don't</h2> | ||
<pre> | ||
[Test] | ||
[TestCase(0, 1, 2, 3, 4, 5)] | ||
public void This_Is_NotValid(int a, int b, int c, int d, int e, int f) | ||
{ | ||
// ... | ||
} | ||
</pre> | ||
|
||
<h2>Do</h2> | ||
<pre> | ||
// By splitting the test into multiple, the test intention becomes more obvious. | ||
|
||
[Test] | ||
[TestCase(0, 1, 2)] | ||
public void This_Is_Valid1(int a, int b, int c) | ||
{ | ||
// ... | ||
} | ||
|
||
[Test] | ||
[TestCase(3, 4, 5)] | ||
public void This_Is_Valid2(int d, int e, int f) | ||
{ | ||
// ... | ||
} | ||
</pre> | ||
# Limit test method parameters to 5 or fewer | ||
|
||
ID: AG0013 | ||
|
||
Type: Code Smell | ||
|
||
Test methods with too many input parameters become difficult to understand and maintain. When test cases need many parameters, split them into smaller, more focused tests with clear intentions. | ||
|
||
#### Don't | ||
|
||
```csharp | ||
[Test] | ||
[TestCase(0, 1, 2, 3, 4, 5)] // Noncompliant - too many parameters | ||
public void This_Is_NotValid(int a, int b, int c, int d, int e, int f) | ||
{ | ||
// Test becomes hard to understand | ||
} | ||
``` | ||
|
||
#### Do | ||
|
||
```csharp | ||
[Test] | ||
[TestCase(0, 1, 2)] | ||
public void This_Is_Valid1(int a, int b, int c) | ||
{ | ||
// Clear test purpose | ||
} | ||
|
||
[Test] | ||
[TestCase(3, 4, 5)] | ||
public void This_Is_Valid2(int d, int e, int f) | ||
{ | ||
// Clear test purpose | ||
} | ||
``` | ||
|
||
Having too many test parameters creates problems: | ||
|
||
- Makes test intention unclear | ||
- Leads to combinatorial explosion of test cases | ||
- Harder to maintain and modify | ||
- Difficult to understand test failures | ||
- Reduces test readability | ||
- Makes refactoring more difficult | ||
- Can hide logical groupings of test cases | ||
|
||
Instead: | ||
|
||
- Split into multiple focused tests | ||
- Use test helper methods | ||
- Create test data builders | ||
- Use meaningful parameter names | ||
- Group related parameters into objects | ||
|
||
This keeps tests clear, maintainable, and easier to understand. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,23 +1,85 @@ | ||
# Rule AG0023 | ||
|
||
<p> | ||
Use Task.Delay instead of Thread.Sleep | ||
</p> | ||
|
||
<p> | ||
<ul> | ||
<li>Thread.Sleep will block the current thread</li> | ||
<li>blocking threads has negative impact on our servers capacity, scalability and performance</li> | ||
<li>using Task.Delay will pause your execution but will free the thread to do other stuff.</li> | ||
</ul> | ||
</p> | ||
|
||
<h2>Noncompliant Code Examples</h2> | ||
<pre> | ||
Thread.Sleep(500); | ||
</pre> | ||
|
||
<h2>Compliant Code Example</h2> | ||
<pre> | ||
await Task.Delay(500); | ||
</pre> | ||
# Use Task.Delay Instead of Thread.Sleep | ||
|
||
ID: AG0023 | ||
|
||
Type: Performance / Code Smell | ||
|
||
## Summary | ||
|
||
Never use `Thread.Sleep()` for delays in code. Instead, use the asynchronous `Task.Delay()` to avoid blocking threads. | ||
|
||
## Explanation | ||
|
||
`Thread.Sleep()` blocks the current thread, preventing it from doing any work during the sleep period. This has several negative impacts: | ||
- Reduces server capacity | ||
- Decreases application scalability | ||
- Degrades overall performance | ||
- Wastes thread pool resources | ||
- Can cause deadlocks in UI applications | ||
|
||
Using `Task.Delay()` allows the thread to handle other work while waiting for the delay to complete. | ||
|
||
### Don't ❌ | ||
|
||
```csharp | ||
public void ProcessWithDelay() | ||
{ | ||
// Do something | ||
Thread.Sleep(1000); // Blocks thread for 1 second | ||
// Do more stuff | ||
} | ||
|
||
public void RetryOperation() | ||
{ | ||
for (int i = 0; i < 3; i++) | ||
{ | ||
try | ||
{ | ||
// Do operation | ||
return; | ||
} | ||
catch | ||
{ | ||
Thread.Sleep(500); // Bad! Blocks thread between retries | ||
} | ||
} | ||
} | ||
``` | ||
|
||
### Do ✅ | ||
|
||
```csharp | ||
public async Task ProcessWithDelayAsync() | ||
{ | ||
// Do something | ||
await Task.Delay(1000); // Thread remains free | ||
// Do more stuff | ||
} | ||
|
||
public async Task RetryOperationAsync() | ||
{ | ||
for (int i = 0; i < 3; i++) | ||
{ | ||
try | ||
{ | ||
// Do operation | ||
return; | ||
} | ||
catch | ||
{ | ||
await Task.Delay(500); // Good! Thread can do other work | ||
} | ||
} | ||
} | ||
``` | ||
|
||
## Why Use Task.Delay? | ||
|
||
- Maintains thread pool efficiency | ||
- Improves application responsiveness | ||
- Better resource utilization | ||
- Prevents UI freezing | ||
- Scales better under load | ||
- Follows async/await best practices | ||
|
||
Remember: There's never a good reason to use `Thread.Sleep()` in modern .NET applications. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,23 +1,76 @@ | ||
# Rule AG0025 | ||
|
||
<p> | ||
Use await instead of Task.ContinueWith | ||
</p> | ||
|
||
<p> | ||
<ul> | ||
<li>Task.ContinueWith has some surprising and non-intuitive behaviors, so must be used with care.</li> | ||
<li>await has none of these problems. It is also more readable.</li> | ||
<li>Therefore, Task.ContinueWith should not be used unless you are doing dynamic task parallelism, which is rare.</li> | ||
</ul> | ||
</p> | ||
|
||
<h2>Noncompliant Code Example</h2> | ||
<pre> | ||
await downloadTask.ContinueWith(async t => await SaveFileAsync(t.Result)); | ||
</pre> | ||
|
||
<h2>Compliant Code Example</h2> | ||
<pre> | ||
await SaveFileAsync(await downloadTask); | ||
</pre> | ||
# Use await Instead of Task.ContinueWith | ||
|
||
ID: AG0025 | ||
|
||
Type: Code Smell / Best Practice | ||
|
||
## Summary | ||
|
||
Avoid using `Task.ContinueWith` as it has subtle and non-intuitive behaviors. Use the `await` keyword instead, which is clearer and safer. | ||
|
||
## Explanation | ||
|
||
`Task.ContinueWith` comes with several pitfalls: | ||
|
||
- Unexpected exception handling behavior | ||
- Complex task scheduling rules | ||
- Unclear execution context flow | ||
- Harder to read and maintain | ||
- Can lead to deadlocks if not used carefully | ||
- More prone to error than `await` | ||
|
||
Only use `Task.ContinueWith` for dynamic task parallelism scenarios, which are rare. | ||
|
||
### Don't ❌ | ||
|
||
```csharp | ||
// Confusing and error-prone | ||
await downloadTask.ContinueWith(async t => await SaveFileAsync(t.Result)); | ||
|
||
// Exception handling is tricky | ||
task.ContinueWith(t => | ||
{ | ||
if (t.IsFaulted) | ||
HandleException(t.Exception); | ||
else | ||
ProcessResult(t.Result); | ||
}); | ||
|
||
// Complicated continuation chains | ||
task.ContinueWith(t => DoSomething()) | ||
.ContinueWith(t => DoSomethingElse()); | ||
``` | ||
|
||
### Do ✅ | ||
|
||
```csharp | ||
// Clear and straightforward | ||
await SaveFileAsync(await downloadTask); | ||
|
||
// Clean exception handling | ||
try | ||
{ | ||
var result = await task; | ||
ProcessResult(result); | ||
} | ||
catch (Exception ex) | ||
{ | ||
HandleException(ex); | ||
} | ||
|
||
// Easy to understand flow | ||
var result1 = await DoSomething(); | ||
var result2 = await DoSomethingElse(); | ||
``` | ||
|
||
## Why Avoid Task.ContinueWith? | ||
|
||
- More complex than necessary | ||
- Easy to misuse | ||
- Poor exception handling | ||
- Confusing task scheduling behavior | ||
- Hard to reason about execution context | ||
- Less readable and maintainable | ||
- `await` provides cleaner, safer alternatives | ||
|
||
Remember: Use `await` by default. Only consider `Task.ContinueWith` for specific dynamic task parallelism scenarios. |
Oops, something went wrong.