From 737d5b03c02295b764e3bf55e456beaf372b94ac Mon Sep 17 00:00:00 2001 From: jdickson Date: Sat, 14 Dec 2024 12:27:27 +0700 Subject: [PATCH] updates from review comments --- doc/AG0001.md | 4 ++-- doc/AG0002.md | 4 ++-- doc/AG0003.md | 4 ++-- doc/AG0004.md | 4 ++-- doc/AG0005.md | 6 +++--- doc/AG0006.md | 4 +--- doc/AG0009.md | 4 ++-- doc/AG0010.md | 6 +++--- doc/AG0011.md | 4 ++-- doc/AG0012.md | 4 ++-- doc/AG0013.md | 7 +++---- doc/AG0018.md | 2 +- doc/AG0019.md | 3 ++- 13 files changed, 27 insertions(+), 29 deletions(-) diff --git a/doc/AG0001.md b/doc/AG0001.md index 6f2ad01..d9749ed 100644 --- a/doc/AG0001.md +++ b/doc/AG0001.md @@ -12,13 +12,13 @@ Direct usage of `DependencyResolver` creates tight coupling and makes code harde - Improved maintainability by making dependencies visible - Stronger adherence to SOLID principles, particularly Dependency Inversion -#### Don't +#### Don't ❌ ```c# var exampleService = DependencyResolver.Current.GetService(); ``` -#### Do +#### Do ✅ ```c# public class ExampleClass diff --git a/doc/AG0002.md b/doc/AG0002.md index 0fec37c..bdb6459 100644 --- a/doc/AG0002.md +++ b/doc/AG0002.md @@ -6,7 +6,7 @@ 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 +#### Don't ❌ ```csharp interface ISomething @@ -25,7 +25,7 @@ class TestClass : ISomething } ``` -#### Do +#### Do ✅ ```csharp interface ISomething diff --git a/doc/AG0003.md b/doc/AG0003.md index 7ab9c86..2b43e7e 100644 --- a/doc/AG0003.md +++ b/doc/AG0003.md @@ -8,7 +8,7 @@ https://agoda-com.github.io/standards-c-sharp/services/framework-abstractions.ht 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 +#### Don't ❌ ```csharp using System.Web; @@ -32,7 +32,7 @@ class TestClass: ISomething } ``` -#### Do +#### Do ✅ ```csharp using System.Web; diff --git a/doc/AG0004.md b/doc/AG0004.md index ba97c97..531087c 100644 --- a/doc/AG0004.md +++ b/doc/AG0004.md @@ -8,7 +8,7 @@ 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 +#### Don't ❌ ```csharp // Both fail at runtime after namespace changes @@ -16,7 +16,7 @@ var instance = Activator.CreateInstance("Agoda", "Agoda.MyType"); var type = Type.GetType("Agoda.MyType"); ``` -#### Do +#### Do ✅ ```csharp // Caught at compile time after namespace changes diff --git a/doc/AG0005.md b/doc/AG0005.md index 2e0c187..99734eb 100644 --- a/doc/AG0005.md +++ b/doc/AG0005.md @@ -13,7 +13,7 @@ Test names should follow this pattern: - Class: `Tests` - Methods: `__` or `_` -#### Don't +#### Don't ❌ ```csharp [Test] @@ -21,7 +21,7 @@ public void HazardLightsTest() // Noncompliant - unclear what aspect is being te {...} ``` -#### Do +#### Do ✅ ```csharp [Test] @@ -43,4 +43,4 @@ If naming a test is difficult, it might indicate the test is doing too much and - Easy identification of failures - Documentation of behavior - Coverage visibility -- Maintainable test suite \ No newline at end of file +- Maintainable test suite diff --git a/doc/AG0006.md b/doc/AG0006.md index 8fe5a55..512a0e6 100644 --- a/doc/AG0006.md +++ b/doc/AG0006.md @@ -6,11 +6,9 @@ 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. -This is a blocker rule - any component with multiple public constructors should be refactored to have only one. - Reasons to avoid multiple public constructors: -- Creates ambiguity for the DI container when resolving dependencies +- 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 diff --git a/doc/AG0009.md b/doc/AG0009.md index 727eee1..1413b3e 100644 --- a/doc/AG0009.md +++ b/doc/AG0009.md @@ -6,7 +6,7 @@ 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 +#### Don't ❌ ```csharp using Microsoft.AspNetCore.Http; @@ -31,7 +31,7 @@ class TestClass : ISomething } ``` -#### Do +#### Do ✅ ```csharp interface ISomething diff --git a/doc/AG0010.md b/doc/AG0010.md index 462b95b..a1dc0de 100644 --- a/doc/AG0010.md +++ b/doc/AG0010.md @@ -8,7 +8,7 @@ https://agoda-com.github.io/standards-c-sharp/unit-testing/be-wary-of-refactorin 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 +#### Don't ❌ ```csharp [TestFixture] @@ -18,7 +18,7 @@ public class TestClass : BaseTest // Noncompliant - inheriting from base test c } ``` -#### Do +#### Do ✅ ```csharp [TestFixture] @@ -46,4 +46,4 @@ Instead of inheritance, prefer: - Shared test data factories - Clear, explicit test setup within each test class -This keeps tests more maintainable and easier to understand at a glance. \ No newline at end of file +This keeps tests more maintainable and easier to understand at a glance. diff --git a/doc/AG0011.md b/doc/AG0011.md index 1414a02..a2d1017 100644 --- a/doc/AG0011.md +++ b/doc/AG0011.md @@ -8,7 +8,7 @@ https://agoda-com.github.io/standards-c-sharp/services/framework-abstractions.ht Direct access to `QueryString` collection bypasses ASP.NET model binding, which provides validation, type conversion, and security features. Always use model binding with action method parameters instead. -#### Don't +#### Don't ❌ ```csharp public ActionResult Index() @@ -18,7 +18,7 @@ public ActionResult Index() } ``` -#### Do +#### Do ✅ ```csharp public ActionResult Index([FromQuery] int id) diff --git a/doc/AG0012.md b/doc/AG0012.md index 387e9c2..2941001 100644 --- a/doc/AG0012.md +++ b/doc/AG0012.md @@ -8,7 +8,7 @@ https://agoda-com.github.io/standards-c-sharp/testing/tests-as-a-specification.h Each test method should contain at least one assertion to verify expected behavior. Tests without assertions don't validate anything and provide a false sense of security. -#### Don't +#### Don't ❌ ```csharp using NUnit.Framework; @@ -23,7 +23,7 @@ public class TestClass } ``` -#### Do +#### Do ✅ With NUnit: diff --git a/doc/AG0013.md b/doc/AG0013.md index 604775c..8b8c469 100644 --- a/doc/AG0013.md +++ b/doc/AG0013.md @@ -8,7 +8,7 @@ https://agoda-com.github.io/standards-c-sharp/unit-testing/use-test-cases-approp 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 +#### Don't ❌ ```csharp [Test] @@ -19,7 +19,7 @@ public void This_Is_NotValid(int a, int b, int c, int d, int e, int f) } ``` -#### Do +#### Do ✅ ```csharp [Test] @@ -50,9 +50,8 @@ Having too many test parameters creates problems: 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. \ No newline at end of file +This keeps tests clear, maintainable, and easier to understand. diff --git a/doc/AG0018.md b/doc/AG0018.md index 0f84a8e..04736f9 100644 --- a/doc/AG0018.md +++ b/doc/AG0018.md @@ -17,7 +17,7 @@ When exposing collections through public APIs, you should return interface types ### Allowed Return Types - `IEnumerable` -- `ISet` +- `ISet` - `IList` - `IDictionary` - `IReadOnlyDictionary` diff --git a/doc/AG0019.md b/doc/AG0019.md index 3b81d2b..4b3b103 100644 --- a/doc/AG0019.md +++ b/doc/AG0019.md @@ -59,5 +59,6 @@ Using `InternalsVisibleTo` leads to several problems: - Makes refactoring more difficult as tests depend on internal structure - Violates encapsulation principles - Can hide poor API design by allowing tests to bypass public interfaces +- Avoids you doing whitebox testing, and focuses you more towards blackbox testing which leads to easier refactoring of code in the long run -Focus on testing behavior through public interfaces rather than implementation details through internal members. \ No newline at end of file +Focus on testing behavior through public interfaces rather than implementation details through internal members.