You can not select more than 25 topics Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.

102 lines
4.4 KiB

  1. # Coding Best Practices
  2. This document describes some typical programming pitfalls and best practices
  3. related to C# and .NET.  It will grow and change as we encounter new situations
  4. and the codebase evolves.
  5. ## Correctness & Business Logic
  6. - Validate if the code adheres to what it is supposed to do. Compare the
  7. implementation with the specification and ask questions, to get clarity.
  8. - Employ `Debug.Assert()` statements to validate program state, but never
  9. rely on them to control program flow. For example, `Debug.Assert()` must
  10. never be used to validate input.
  11. ## Memory / Resource Management
  12. - C# – Rent large buffers from ArrayPool<T> and return them promptly to
  13. cut down on heap allocations and improve throughput. Have the buffers been
  14. returned to the pool? Will they be returned if an exception is thrown?
  15. - Prefer Span<T> for stack allocated buffers.
  16. - Double check the depth of the call stack if using async/await. Stack depth of
  17. unwinding async/await state-machines of more than 3 calls can cause
  18. performance issues.
  19. - Resource cleanup Dispose(), Close(), or the equivalent to release files,
  20. sockets, database connections, and other external resources.
  21. ## Error Handling & Reliability 
  22. - Prefer return values to throwing exceptions.  Returns are strongly typed and
  23. checked by the compiler. Consider patterns like
  24. `bool TryXYZ(..., out Result result)` instead of `Result XYZ(...)`.
  25. - Exceptions should be reserved for exceptional circumstances, like constructor
  26. failure or out-of-memory, where the calling code stands little chance of
  27. recovering. Routine, expected error conditions should not be expressed as
  28. exceptions.
  29. - Document all exceptions that may be thrown from an API.
  30. - Do not let undocumented exceptions escape from an API.
  31. - Be permissive with what you accept, and strict with what you produce.
  32. - Consider Constrained Execution Regions for .NET Framework implementations.
  33. ## Async, Concurrency & Thread Safety (if applicable)
  34. - Avoid calling Task.Result, Task.Wait() or Task.GetAwaiter().GetResult(). If
  35. needed, exercise extreme caution. These can cause deadlocks.
  36. - When awaiting a task with the await keyword, always use
  37. Task.ConfigureAwait(false) so that the task continuation may run on any thread
  38. rather than marshalling back to the original calling thread.
  39. ## Backward Compatibility
  40. - Someone will always depend on your undocumented behaviour.  It is just as much
  41. an API promise as explicitly documented behaviour.
  42. - Use language features to deprecate/obsolete APIs, and to issue warnings for
  43. notable upcoming breaking changes.
  44. ## Security Considerations
  45. - Never log passwords, secrets, or connection strings with credentials.
  46. - Validate inputs to avoid SQL injection, even on metadata calls.
  47. - Are there any user inputs going into SQL or shell commands directly?
  48. - Are secrets being logged or exposed in stack traces?
  49. - Are TLS/certificate settings handled safely and explicitly?
  50. - Are we sending unbounded data streams to server prior to authentication e.g.
  51. in feature extensions?
  52. ## Performance & Scalability 
  53. - For major features or large PRs, always run the internal performance
  54. benchmarks or performance testsuite to determine if the new changes are
  55. causing any performance degradation.
  56. ## Observability (Logging / Tracing / Metrics) 
  57. - Error messages should contain enough context to understand the state of the
  58. program and find the source code responsible for emitting it.
  59. - Instrument changes for metrics/tracing/logging to aid future field diagnosis,
  60. debugging, and app behaviour analysis.
  61. ## Unit Tests / Integration
  62. - Have you added unit tests and integration tests?
  63. - Unit tests should not depend on external resources.  Code under unit test
  64. should permit mocked resources to be provided/injected.
  65. - Avoid tests that rely on timing.
  66. ## Configuration & Feature Flags 
  67. - Is the change considered a breaking change? If breaking change is not
  68. avoidable, has a Configuration/Feature flag been introduced for customers to
  69. revert to old behavior?
  70. ## Code Coverage expectations
  71. - Ideally these expectations  should be codified into scanning tool
  72. configuration that lives in the repo.
  73. - Specific metrics will be chosen based on realistic analysis of existing
  74. coverage and will be re-evaluated as testing matures.
  75. ## Pipeline runs
  76. - Is the CI passing? If not, then have you looked at the failures, and found the
  77. cause for failures?