Make private fields readonly when possible #2103
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2103 +/- ##
=======================================
Coverage 79.65% 79.65%
=======================================
Files 233 233
Lines 7325 7325
=======================================
Hits 5835 5835
Misses 1490 1490
|
| @@ -30,8 +30,8 @@ namespace Examples.Console | |||
| internal class InstrumentationWithActivitySource : IDisposable | |||
| { | |||
| private const string RequestPath = "/api/request"; | |||
| private SampleServer server = new SampleServer(); | |||
| private SampleClient client = new SampleClient(); | |||
| private readonly SampleServer server = new SampleServer(); | |||
There was a problem hiding this comment.
Curious, do we consider this as pedantic for example code?
| @@ -24,7 +24,7 @@ namespace OpenTelemetry | |||
| { | |||
| public class CompositeProcessor<T> : BaseProcessor<T> | |||
| { | |||
| private DoublyLinkedListNode head; | |||
| private readonly DoublyLinkedListNode head; | |||
There was a problem hiding this comment.
I want to get more perspectives regarding this one.
Making it readonly makes me feel that we're assuming the head should never change, and my worry is that other code might assume it (e.g. they might cache the value and assume it will never change).
While this is true for now, I guess in the future we might want to support something like PrependProcessor(processor) or InsertProcessor(index, processor).
There was a problem hiding this comment.
@CodeBlanch do you know if the common practice in C# is to mark something readonly due to the semantics (e.g. we know that it will never change) or due to the actual usage (e.g. the fact that we didn't change it after initialization)?
To my knowledge in C++ it is focusing on semantics rather than the actual usage (e.g. const/mutable/volatile).
There was a problem hiding this comment.
@reyang I would say in C# it is based on actual usage. There is an IDE analyzer built into VS that suggests/warns about this case if it can tell you aren't modifying a private field. So you would have to go out of your way/fight the tooling (suppression, disable rule) to stick with the semantic approach.
There was a problem hiding this comment.
I want to get more perspectives regarding this one.
Making itreadonlymakes me feel that we're assuming the head should never change, and my worry is that other code might assume it (e.g. they might cache the value and assume it will never change).
While this is true for now, I guess in the future we might want to support something likePrependProcessor(processor)orInsertProcessor(index, processor).
As of now, the processors get added in the order of the AddProcessor calls. Would we need to offer something like PrependProcessor or InsertProcessor if we can just ask the user to order their AddProcessor calls accordingly?
There was a problem hiding this comment.
As of now, the processors get added in the order of the
AddProcessorcalls. Would we need to offer something likePrependProcessororInsertProcessorif we can just ask the user to order theirAddProcessorcalls accordingly?
I think in the world where a single user/dev/team controls everything, this should work, and is not an issue from the beginning.
In the real world, there might be separation of roles/responsibilities. For example you have an infrastructure team which developed several security or privacy related processors, and that team wants to "prepend" the security/privacy processor before other processors introduced by feature teams. This can be solved by having the infrastructure team establishing some rules (e.g. all the teams should call AddInfrastructureProcessors before adding their stuff), but practically it might be challenging.
|
@utpilla Another thing that might be worth doing: seal internal/private types. |
Changes