search

launchdarkly / dotnet-sdk-common

Code shared between dotnet-server-sdk and dotnet-client-sdk
Other
3 stars 10 forks source link

Private Setters #34

Closed JeffAshton closed 5 years ago

JeffAshton commented 5 years ago

Is there a particular reason you are using private setters still? Instead of just omitting the setter?

For example:

https://github.com/launchdarkly/dotnet-sdk-common/blob/master/src/LaunchDarkly.CommonSdk/Public/Event.cs#L51

When you omit the setter entirely, the compiler enforces that the value can only be set in the constructor, which aligns with your usage. Was this just grandfathered in?

eli-darkly commented 5 years ago

Probably; this comes from very old code that has barely changed since the first version.

Can you say more about why you raised this as an issue?

JeffAshton commented 5 years ago

Immutability. Using the readonly properties ensures Immutability of these objects (assuming no reflection). Reduces likelihood of bugs.

eli-darkly commented 5 years ago

Well, I agree that it makes sense to minimize such possibility, but just to be clear, they are private setters so (except for reflection) no code outside of the same class can modify them—and there is basically no code in those classes.

JeffAshton commented 5 years ago

Even less code without the setters. :) Generally when I open a class, I let the modifiers communicate the intent. If these classes are in fact intended to be immutable, then by omitting the setters, the compiler will ensure this, instead of relying on the humans. And the less mutable classes, the less concurrency issues I have to worry about.

eli-darkly commented 5 years ago

May I close this issue, since your question has been answered? I'll take it into account during ongoing code cleanup.

JeffAshton commented 5 years ago

Sounds good. Just wanted to make sure you were aware of the difference.