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.

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

Replace public fields with properties #1419

Merged
merged 1 commit into from
Dec 11, 2018

Conversation

ob-stripe
Copy link
Contributor

r? @remi-stripe
cc @stripe/api-libraries

Replace public fields by private fields encapsulated by public properties.

Public fields in public classes do not respect the encapsulation principle and has three main disadvantages:

Additional behavior such as validation cannot be added.
The internal representation is exposed, and cannot be changed afterwards.
Member values are subject to change from anywhere in the code and may not meet the programmer's assumptions.
By using private fields and public properties (set and get), unauthorized modifications are prevented. Properties also benefit from additional protection (security) features such as Link Demands.

Note that due to optimizations on simple properties, public fields provide only very little performance gain.

public static string StripeApiVersion
{
get { return stripeApiVersion; }
set { stripeApiVersion = value; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Does that mean anyone can set this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. I also have doubts whether this should be allowed, but wanted to maintain the current behavior.

@remi-stripe remi-stripe assigned ob-stripe and unassigned remi-stripe Dec 11, 2018
@ob-stripe ob-stripe merged commit 08a2dcf into integration-v23 Dec 11, 2018
@ob-stripe ob-stripe deleted the ob-replace-public-fields-with-properties branch December 11, 2018 23:19
@ob-stripe ob-stripe mentioned this pull request Dec 12, 2018
29 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants