-
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.
Revise rules and being to add meta data (#200)
* Revise rules and being to add meta data * Add failing test * more updates to docs * passing tests * Update all docs links * updates from review comments * using clean up --------- Co-authored-by: jdickson <[email protected]>
- Loading branch information
1 parent
5f5ecf7
commit 526887a
Showing
133 changed files
with
2,690 additions
and
1,495 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 |
---|---|---|
@@ -0,0 +1,37 @@ | ||
# Avoid direct usage of DependencyResolver | ||
|
||
ID: AG0001 | ||
|
||
Type: Code Smell | ||
|
||
https://agoda-com.github.io/standards-c-sharp/di/attribute-based-registration.html | ||
|
||
Direct usage of `DependencyResolver` creates tight coupling and makes code harder to test. Dependencies should be explicitly declared through constructor injection, which promotes: | ||
|
||
- 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 |
---|---|---|
@@ -0,0 +1,51 @@ | ||
# Classes should only expose functionality used in their public interface | ||
|
||
ID: AG0002 | ||
|
||
Type: Code Smell | ||
|
||
When implementing an interface, classes should only expose methods that are part of their public contract. Additional public or internal methods that aren't part of the implemented interfaces create confusion about the class's responsibilities and violate the Interface Segregation Principle. | ||
|
||
#### Don't ❌ | ||
|
||
```csharp | ||
interface ISomething | ||
{ | ||
void DoSomething(); | ||
} | ||
|
||
class TestClass : ISomething | ||
{ | ||
public void DoSomething() | ||
{ | ||
} | ||
internal void DoSomething2() // Noncompliant - not part of interface | ||
{ | ||
} | ||
} | ||
``` | ||
|
||
#### Do ✅ | ||
|
||
```csharp | ||
interface ISomething | ||
{ | ||
void DoSomething(); | ||
} | ||
|
||
class TestClass : ISomething | ||
{ | ||
public void DoSomething() | ||
{ | ||
} | ||
} | ||
``` | ||
|
||
Adding methods that aren't part of the interface creates several problems: | ||
|
||
- Violates Interface Segregation Principle by potentially forcing clients to depend on methods they don't use | ||
- Makes the code harder to understand and maintain by obscuring the class's true responsibilities | ||
- Can lead to tight coupling if these additional methods are used by other classes | ||
- Makes testing more difficult as you need to consider methods outside the interface contract | ||
|
||
If additional functionality is needed, consider creating a new interface that better represents the complete contract of the class. |
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 |
---|---|---|
@@ -0,0 +1,67 @@ | ||
# Avoid passing HttpContext as a parameter | ||
|
||
ID: AG0003 | ||
|
||
Type: Code Smell | ||
|
||
https://agoda-com.github.io/standards-c-sharp/services/framework-abstractions.html | ||
|
||
Passing `System.Web.HttpContext` as a parameter creates tight coupling to the web infrastructure and makes unit testing significantly more difficult. Instead, pass only the specific HttpContext properties that your code actually needs. | ||
|
||
#### Don't ❌ | ||
|
||
```csharp | ||
using System.Web; | ||
|
||
interface ISomething | ||
{ | ||
void SomeMethod(HttpContext c, string sampleString); // Noncompliant | ||
} | ||
|
||
class TestClass: ISomething | ||
{ | ||
public void SomeMethod(HttpContext context, string sampleString) | ||
{ | ||
// Hard to test due to HttpContext dependency | ||
} | ||
|
||
public TestClass(System.Web.HttpContext context) | ||
{ | ||
// Constructor with HttpContext dependency | ||
} | ||
} | ||
``` | ||
|
||
#### Do ✅ | ||
|
||
```csharp | ||
using System.Web; | ||
|
||
interface ISomething | ||
{ | ||
void SomeMethod(string userAgent, string sampleString); | ||
} | ||
|
||
class TestClass: ISomething | ||
{ | ||
public void SomeMethod(string userAgent, string sampleString) | ||
{ | ||
// Only depends on what it needs | ||
} | ||
|
||
public TestClass(string userAgent) | ||
{ | ||
// Clean constructor with specific dependencies | ||
} | ||
} | ||
``` | ||
|
||
Passing HttpContext creates several problems: | ||
|
||
- Makes unit testing difficult since HttpContext is complex to mock | ||
- Violates Single Responsibility Principle by potentially accessing many different context properties | ||
- Creates tight coupling to ASP.NET infrastructure | ||
- Obscures the actual dependencies of your code | ||
- Makes it harder to port code to other platforms or frameworks | ||
|
||
Instead, identify and pass only the specific HttpContext properties your code needs (like UserAgent, Request.Path, etc). This improves testability and makes dependencies explicit. |
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 |
---|---|---|
@@ -0,0 +1,35 @@ | ||
# Avoid hard-coded type strings | ||
|
||
ID: AG0004 | ||
|
||
Type: Bug | ||
|
||
https://agoda-com.github.io/standards-c-sharp/reflection/hard-coded-strings.html | ||
|
||
Hard-coded strings to identify namespaces and types make refactoring risky and move type resolution errors from compile-time to runtime. Always use the `typeof` operator to reference types, which provides compile-time safety. | ||
|
||
#### Don't ❌ | ||
|
||
```csharp | ||
// Both fail at runtime after namespace changes | ||
var instance = Activator.CreateInstance("Agoda", "Agoda.MyType"); | ||
var type = Type.GetType("Agoda.MyType"); | ||
``` | ||
|
||
#### Do ✅ | ||
|
||
```csharp | ||
// Caught at compile time after namespace changes | ||
var instance = Activator.CreateInstance(typeof(Agoda.MyType)); | ||
var type = typeof(Agoda.MyType); | ||
``` | ||
|
||
Using string literals for type identification creates several problems: | ||
|
||
- Refactoring operations like namespace moves or type renames won't update the strings | ||
- Type resolution failures only surface at runtime instead of compile time | ||
- No IntelliSense or IDE support for type names | ||
- More difficult to maintain as type references aren't tracked by development tools | ||
- Can lead to runtime exceptions in production that could have been caught during development | ||
|
||
Always use language features like `typeof()` that provide compile-time type safety and refactoring support. |
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 |
---|---|---|
@@ -0,0 +1,46 @@ | ||
# Test methods should have descriptive names | ||
|
||
ID: AG0005 | ||
|
||
Type: Code Smell | ||
|
||
https://agoda-com.github.io/standards-c-sharp/testing/test-method-names-should-clearly-indicate-what-they-are-testing.html | ||
|
||
Test method names should clearly communicate what is being tested, under what conditions, and the expected outcome. This makes tests serve as documentation and helps quickly identify what failed when tests break. | ||
|
||
Test names should follow this pattern: | ||
|
||
- Class: `<ClassNameUnderTest>Tests` | ||
- Methods: `<SystemUnderTest>_<PreCondition>_<PostCondition>` or `<SystemUnderTest>_<PostCondition>` | ||
|
||
#### Don't ❌ | ||
|
||
```csharp | ||
[Test] | ||
public void HazardLightsTest() // Noncompliant - unclear what aspect is being tested | ||
{...} | ||
``` | ||
|
||
#### Do ✅ | ||
|
||
```csharp | ||
[Test] | ||
public void HazardLightsToggleButton_WhenPushedWithLightsBlinking_StopsLightsBlinking() | ||
{...} | ||
``` | ||
|
||
Poor test names create several problems: | ||
|
||
- Makes it harder to understand test failures without reading the implementation | ||
- Reduces test suite's value as documentation | ||
- Makes it difficult to identify missing test cases | ||
- Complicates test maintenance and refactoring | ||
- Makes it harder for team members to understand test coverage | ||
|
||
If naming a test is difficult, it might indicate the test is doing too much and should be split into more focused tests. Good test names help ensure: | ||
|
||
- Clear test purpose | ||
- Easy identification of failures | ||
- Documentation of behavior | ||
- Coverage visibility | ||
- Maintainable test suite |
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 |
---|---|---|
@@ -0,0 +1,39 @@ | ||
# Components should have a single public constructor | ||
|
||
ID: AG0006 | ||
|
||
Type: Code Smell | ||
|
||
Each component registered with the dependency injection container should have exactly one public constructor. Having multiple public constructors leads to ambiguity in dependency resolution and makes the codebase harder to maintain. | ||
|
||
Reasons to avoid multiple public constructors: | ||
|
||
- Creates ambiguity for the DI container when resolving dependencies | ||
- Makes it harder to track and manage dependencies | ||
- Increases complexity of dependency resolution | ||
- Makes code harder to test and maintain | ||
- Can lead to runtime errors if wrong constructor is chosen | ||
- Violates the Single Responsibility Principle | ||
|
||
If a dependency is truly optional, use the null object pattern instead of multiple constructors: | ||
|
||
```csharp | ||
// Don't do this | ||
public class Service | ||
{ | ||
public Service() { } // Noncompliant - multiple constructors | ||
public Service(ILogger logger) { } | ||
} | ||
|
||
// Do this instead | ||
public class Service | ||
{ | ||
public Service(ILogger logger = null) // Single constructor with optional dependency | ||
{ | ||
_logger = logger ?? NullLogger.Instance; | ||
} | ||
} | ||
``` | ||
|
||
Always prefer explicit dependency declaration through a single constructor for clearer and more maintainable code. |
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 |
---|---|---|
@@ -0,0 +1,64 @@ | ||
# Avoid passing HttpContextAccessor as a parameter | ||
|
||
ID: AG0009 | ||
|
||
Type: Code Smell | ||
|
||
Passing `IHttpContextAccessor` or `HttpContextAccessor` as a parameter creates tight coupling to ASP.NET Core infrastructure and makes testing difficult. Instead, pass only the specific HTTP context properties that your code actually needs. | ||
|
||
#### Don't ❌ | ||
|
||
```csharp | ||
using Microsoft.AspNetCore.Http; | ||
|
||
interface ISomething | ||
{ | ||
void SomeMethod(IHttpContextAccessor c, string sampleString); // Noncompliant | ||
void SomeMethod(HttpContextAccessor c, string sampleString); // Noncompliant | ||
} | ||
|
||
class TestClass : ISomething | ||
{ | ||
public void SomeMethod(IHttpContextAccessor context, string sampleString) | ||
{ | ||
// Hard to test due to IHttpContextAccessor dependency | ||
} | ||
|
||
public TestClass(IHttpContextAccessor context) | ||
{ | ||
// Constructor with unnecessary dependency | ||
} | ||
} | ||
``` | ||
|
||
#### Do ✅ | ||
|
||
```csharp | ||
interface ISomething | ||
{ | ||
void SomeMethod(string userAgent, string sampleString); | ||
} | ||
|
||
class TestClass : ISomething | ||
{ | ||
public void SomeMethod(string userAgent, string sampleString) | ||
{ | ||
// Only depends on what it needs | ||
} | ||
|
||
public TestClass(string userAgent) | ||
{ | ||
// Clean constructor with specific dependencies | ||
} | ||
} | ||
``` | ||
|
||
Passing HttpContextAccessor creates several problems: | ||
|
||
- Makes unit testing difficult since context is complex to mock | ||
- Violates Single Responsibility Principle by potentially accessing many different context properties | ||
- Creates tight coupling to ASP.NET Core infrastructure | ||
- Obscures the actual dependencies of your code | ||
- Makes it harder to port code to other platforms or frameworks | ||
|
||
Instead, identify and pass only the specific context properties your code needs (like UserAgent, Request.Path, etc). This improves testability and makes dependencies explicit. |
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 |
---|---|---|
@@ -0,0 +1,49 @@ | ||
# Avoid test fixture inheritance | ||
|
||
ID: AG0010 | ||
|
||
Type: Code Smell | ||
|
||
https://agoda-com.github.io/standards-c-sharp/unit-testing/be-wary-of-refactoring-tests.html | ||
|
||
Test fixture inheritance should be avoided as it creates hidden dependencies, makes tests harder to understand, and violates the test isolation principle. Each test class should be independent and self-contained. | ||
|
||
#### Don't ❌ | ||
|
||
```csharp | ||
[TestFixture] | ||
public class TestClass : BaseTest // Noncompliant - inheriting from base test class | ||
{ | ||
// Code | ||
} | ||
``` | ||
|
||
#### Do ✅ | ||
|
||
```csharp | ||
[TestFixture] | ||
public class TestClass | ||
{ | ||
// Code | ||
} | ||
``` | ||
|
||
Test fixture inheritance creates several problems: | ||
|
||
- Makes test dependencies and setup harder to understand | ||
- Complicates test maintenance due to hidden inherited behavior | ||
- Can lead to unexpected test interactions | ||
- Violates test isolation principle | ||
- Makes it difficult to refactor tests | ||
- Can cause setup/teardown ordering issues | ||
- Reduces test clarity and readability | ||
|
||
Instead of inheritance, prefer: | ||
|
||
- Composition through helper methods | ||
- Test utility classes | ||
- Builder patterns | ||
- Shared test data factories | ||
- Clear, explicit test setup within each test class | ||
|
||
This keeps tests more maintainable and easier to understand at a glance. |
Oops, something went wrong.