-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Code Quality: Refactor MeasureOverride method to early exit for null or empty context #17483
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
base: main
Are you sure you want to change the base?
Code Quality: Refactor MeasureOverride method to early exit for null or empty context #17483
Conversation
This PR Prevents throwing a NullReferenceException when context or context.Childrenis null, there's no null checking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While there are some redundant changes made in this file, overall LGTM.
We should keep the look of the code smilar to what MUXC.BreadcrumbBar contrl has.
if (context?.Children == null || context.Children.Count == 0) | ||
{ | ||
return new Size(0, 0); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (context?.Children == null || context.Children.Count == 0) | |
{ | |
return new Size(0, 0); | |
} | |
if (context?.Children is null || context.Children.Count is 0) | |
return new Size(0, 0); |
_availableSize = availableSize; | ||
|
||
var indexAfterEllipsis = GetFirstIndexToRender(context); | ||
var maxHeight = 0; | ||
var totalWidth = 0; | ||
var children = context.Children; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caching like this is redundant.
|
||
return accumulatedSize; | ||
return new Size(totalWidth, maxHeight); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you don't use Size instead, like I did? And I feel like the code changes in the middle are unnecessary, is there a reason why you change there?
Resolved / Related Issues
To prevent extra work, all changes to the Files codebase must link to an approved issue marked as
Ready to build
. Please insert the issue number following the hashtag with the issue number that this Pull Request resolves.Steps used to test these changes
Stability is a top priority for Files and all changes are required to go through testing before being merged into the repo. Please include a list of steps that you used to test this PR.