Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revise rules and begin to add meta data #200

Merged
merged 7 commits into from
Dec 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions doc/AG0001.md
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.
51 changes: 51 additions & 0 deletions doc/AG0002.md
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.
67 changes: 67 additions & 0 deletions doc/AG0003.md
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.
35 changes: 35 additions & 0 deletions doc/AG0004.md
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.
46 changes: 46 additions & 0 deletions doc/AG0005.md
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
39 changes: 39 additions & 0 deletions doc/AG0006.md
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.
64 changes: 64 additions & 0 deletions doc/AG0009.md
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.
49 changes: 49 additions & 0 deletions doc/AG0010.md
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.
Loading
Loading