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.

Sign up for GitHub

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

Make private fields readonly when possible #2103

Merged
merged 8 commits into from
Sep 4, 2021
+11 −11
Merged

Make private fields readonly when possible #2103

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
56afb47
Make private fields readonly where possible
utpilla Jun 24, 2021
1859b24
Rename static readonly field to start with a capital letter
utpilla Jun 24, 2021
a26a354
Merge branch 'main' into utpilla/Makr-Private-Fields-ReadOnly-Where-P…
cijothomas Jun 24, 2021
997e9a6
Merge branch 'main' into utpilla/Makr-Private-Fields-ReadOnly-Where-P…
utpilla Aug 3, 2021
235d36e
Merge branch 'main' into utpilla/Makr-Private-Fields-ReadOnly-Where-P…
utpilla Aug 24, 2021
a615285
Merge branch 'main' into utpilla/Makr-Private-Fields-ReadOnly-Where-P…
utpilla Sep 1, 2021
601c58b
Merge branch 'main' into utpilla/Makr-Private-Fields-ReadOnly-Where-P…
utpilla Sep 1, 2021
ea74b00
Merge branch 'main' into utpilla/Makr-Private-Fields-ReadOnly-Where-P…
cijothomas Sep 4, 2021
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
6 changes: 3 additions & 3 deletions examples/Console/InstrumentationWithActivitySource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, do we consider this as pedantic for example code?

private readonly SampleClient client = new SampleClient();

public void Start(ushort port = 19999)
{
Expand All @@ -48,7 +48,7 @@ public void Dispose()

private class SampleServer : IDisposable
{
private HttpListener listener = new HttpListener();
private readonly HttpListener listener = new HttpListener();

public void Start(string url)
{
Expand Down
6 changes: 3 additions & 3 deletions src/OpenTelemetry.Exporter.ZPages/Implementation/ZPagesActivityTracker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ namespace OpenTelemetry.Exporter.ZPages.Implementation
/// </summary>
internal static class ZPagesActivityTracker
{
private static long startTime;
private static readonly long StartTime;

/// <summary>
/// Initializes static members of the <see cref="ZPagesActivityTracker"/> class.
Expand All @@ -44,7 +44,7 @@ static ZPagesActivityTracker()
TotalEndedCount = new Dictionary<string, long>();
TotalErrorCount = new Dictionary<string, long>();
TotalLatency = new Dictionary<string, long>();
startTime = DateTimeOffset.Now.ToUnixTimeMilliseconds();
StartTime = DateTimeOffset.Now.ToUnixTimeMilliseconds();
}

/// <summary>
Expand Down Expand Up @@ -106,7 +106,7 @@ public static void PurgeCurrentMinuteData(object source, ElapsedEventArgs e)
CurrentMinuteList.Clear();

// Remove the stale activity information which is at the end of the list
if (DateTimeOffset.Now.ToUnixTimeMilliseconds() - startTime >= RetentionTime)
if (DateTimeOffset.Now.ToUnixTimeMilliseconds() - StartTime >= RetentionTime)
{
ZQueue.RemoveLast();
}
Expand Down
2 changes: 1 addition & 1 deletion src/OpenTelemetry/CompositeProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ namespace OpenTelemetry
{
public class CompositeProcessor<T> : BaseProcessor<T>
{
private DoublyLinkedListNode head;
private readonly DoublyLinkedListNode head;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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.

private DoublyLinkedListNode tail;
private bool disposed;

Expand Down
2 changes: 1 addition & 1 deletion src/OpenTelemetry/DiagnosticSourceInstrumentation/DiagnosticSourceSubscriber.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,12 @@ namespace OpenTelemetry.Instrumentation
{
internal class DiagnosticSourceSubscriber : IDisposable, IObserver<DiagnosticListener>
{
private readonly List<IDisposable> listenerSubscriptions;
private readonly Func<string, ListenerHandler> handlerFactory;
private readonly Func<DiagnosticListener, bool> diagnosticSourceFilter;
private readonly Func<string, object, object, bool> isEnabledFilter;
private long disposed;
private IDisposable allSourcesSubscription;
private List<IDisposable> listenerSubscriptions;

public DiagnosticSourceSubscriber(
ListenerHandler handler,
Expand Down
2 changes: 1 addition & 1 deletion test/OpenTelemetry.Instrumentation.AspNet.Tests/HttpInListenerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,7 @@ public void Dispose()

private class TestSampler : Sampler
{
private SamplingDecision samplingDecision;
private readonly SamplingDecision samplingDecision;

public TestSampler(SamplingDecision samplingDecision)
{
Expand Down
2 changes: 1 addition & 1 deletion test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -618,7 +618,7 @@ public override void Inject<T>(PropagationContext context, T carrier, Action<T,

private class TestSampler : Sampler
{
private SamplingDecision samplingDecision;
private readonly SamplingDecision samplingDecision;

public TestSampler(SamplingDecision samplingDecision)
{
Expand Down
6 changes: 3 additions & 3 deletions test/OpenTelemetry.Tests/Instrumentation/DiagnosticSourceListenerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ namespace OpenTelemetry.Instrumentation.Tests
public class DiagnosticSourceListenerTest
{
private const string TestSourceName = "TestSourceName";
private DiagnosticSource diagnosticSource;
private TestListenerHandler testListenerHandler;
private DiagnosticSourceSubscriber testDiagnosticSourceSubscriber;
private readonly DiagnosticSource diagnosticSource;
private readonly TestListenerHandler testListenerHandler;
private readonly DiagnosticSourceSubscriber testDiagnosticSourceSubscriber;

public DiagnosticSourceListenerTest()
{
Expand Down