Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Dec 15, 2021

@adegeo, this PR is ready for your review.

The snippets and safe constructor patterns have been updated because:

  • The first snippet was an incomplete example of unsafe constructor patterns.
  • The safe constructor recommendations didn't resolve the problem introduced in the first snippet, as explained below.

In the first existing snippet, MyClass is a derived class that implements the virtual OnPropertyChanged method. MyClass also declares the uninitialized variable _myList (which doesn't exist in the base class). The first snippet demonstrates how a derived class method (OnPropertyChanged), which overrides a base class virtual method, can access _myList before it's initialized. However, this snippet is an incomplete example, because:

  • It doesn't include the metadata-specified property system callbacks.
  • It doesn't demonstrate the order of execution of static/non-static constructors, events, and callbacks in base/derived classes.
  • It isn't reflective of the scenario mentioned in both the introductory paragraph and in the Safe Constructor Patterns section: "your type is used as a base class".

The two other existing snippets convey that member initialization for callbacks should happen in the base class parameterless constructor. However, that approach doesn't solve the problem in the first snippet because_myList doesn't exist in the base class and so can't be initialized from there. Instead, that problem should be addressed in the derived class. Another recommendation was "make sure that callbacks use only other dependency properties", which isn't appropriate for many dependency property scenarios.

Internal preview link

@ghost ghost marked this pull request as ready for review December 15, 2021 23:15
Copy link
Contributor

@adegeo adegeo left a comment

Choose a reason for hiding this comment

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

Minor wording issue

@adegeo adegeo merged commit 96056b1 into dotnet:main Jan 4, 2022
@ghost
Copy link
Author

ghost commented Jan 4, 2022

@adegeo, your change makes sense. Thank you :)

@adegeo adegeo mentioned this pull request Jan 7, 2022
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants