Small-scale refactorings and syntax modernization in Dispatcher-related code

[Lehonti]

Description

• Reduced levels of nesting through if inversion.
• Auto-applied formatting and modernized syntax where appropriate.

Customer Impact

No direct customer impact. Just refactoring.
The code should behave exactly the same as before

Microsoft Reviewers: Open in CodeFlow

[h3xds1nz]

I do not really see the immediate benefit if you're not making any actual changes, and only, unfortunately, introducing breaking changes. Deleting most of the PR template is also not the best approach.

Please don't get me wrong, community contributions are welcome but I'd recommend reading through https://github.com/dotnet/wpf/blob/main/Documentation/acceptance_criteria.md and https://github.com/dotnet/wpf/blob/main/Documentation/contributing.md first.

There isn't really much benefit of pure "refactoring" without a systematic/discussed (agreed) effort, same as abusing CI without at least trying to build locally first which is considered just a good practice.

[h3xds1nz]

And that's a breaking change.

[Lehonti]

Thanks @h3xds1nz for your review and for the links.

You are right that I should try to build locally first, which I will do once I fix certain issues with my local setup.

I will un-seal the class as per your suggestion, but how is sealing the class a breaking change if all of its constructors are internal? I understand that someone could, perhaps, try to inherit it through Reflection.Emit, but do you think anybody would want to do that?

I also agree that there is no immediate benefit from refactoring, but I think paying off some technical debt has long-term benefits.

As for the guidelines, I could probably create an issue discussing whether this kind of PR would be welcome. It is extremely general, though, and not grounded on a specific issue.

Lastly, how would you fill out the fields of the PR template that don't apply to this PR?

I think these are all fair questions, especially considering that I've had PR like these merged to a Microsoft project (namely WinForms) before.

[h3xds1nz]

Well, WPF has a huge technical debt, I'll give you that, and there's indeed an ongoing effort to decrease such.

Again, my point is that making those sort of refactoring changes without a set goal is simply not the best idea. If you were working at the area (files) on some (e.g. perf) improvements, I'm all in to lump these changes in at the same time (not everyone from the community is on that one but that's alright, opinions differ).

Let's say we set out to do refactoring in this area for WPF, then you might have several differently-focused PRs.

1. new succint syntax
2. expression-bodied properties
3. simplifying delegate invocation
4. using null coalescing operators
   ......
   ....
   x) pressing CTRL+K CTRL+D in VS
   etc. etc.

There's actually an ongoing effort in #10270 to process all the analyzer-related warnings (forgot to mention that last time, sorry.) which is imho a solid start where you could help contribute if interested.

But why exactly you chose to do it in Dispatcher-related classes only, but all those changes at once without making any actual changes other than refactoring, I do not understand. It is a pain to review with the commit history (at least for me). Possibly the PR template could have helped with the description of that. I prefer more descriptive approach than less.

Even then, you've for example made _operation in DispatcherOperation readonly, which is great, now JIT can treat it as constant but why was _waitTimer left behind? The field is not adjusted outside constructor. Hence why I'm saying that a focus on certain things is better than making "1 big refactor" which is not consistent with itself.

Also I do not particularly see how we've improved readibility in Yield method from Dispatcher file.

Properties like Reserved0 instead of being consistently in the old property syntax now use bodied get but the old syntax for set. Leaves me kinda puzzled that the property now uses two different syntaxes.

Refactoring out param as out System.Int32 intResult instead of out intResult which is not declared in-line, sure, but uses int correctly (keywords are preferred) is also more of an L (guess that's the expression kids use nowadays).

Generally is null / is not null is preferred over == null and != null, that's a legacy syntax. I'd welcome a description on why we chose to not include it here.

Next thing are those if-inversions to remove nesting. Excessive nesting is bad, sure, I hate it too. But inverting conditionals and introducing different control flow mostly in loops often carries changes in performance and generated code, e.g. it can actually add branches so you're changing the characteristics of the code as well. Have you evaluated the impact in the loops?

I could go on but I'm not keen on it, let's move on.

Regarding the template, well, I don't see how "Testing" gets to be excluded from the PR template for example as you've inverted a lot of checks and added a new control flow statements that may adjust the code's behaviour.

There's a lot of tech-debt as we've established so we can't rely on everything being covered by unit tests. Nobody's telling you to write them but I guess there was no testing besides CI, not even a local build. In case you have issues running/building locally, you may open an issue so we can assist you if its something with the solution/tooling.

Lastly, you're right that the DispatcherOperation<TResult> could actually be sealed in this case, I retract that bit; we could have established that in the description of the PR though.

---

tl;dr I welcome pure refactoring but a it should be a targeted effort imho. It is easier to review and leaves the codebase (at least on assembly-level basis) in a more consistent state in general. Right now the PR is not even consistent with itself at all with the changes made.

[codecov]

Codecov Report

Attention: Patch coverage is 47.58621% with 228 lines in your changes missing coverage. Please review.

Project coverage is 11.40001%. Comparing base (b360342) to head (844d32f).
Report is 3 commits behind head on main.

Additional details and impacted files

@@                 Coverage Diff                 @@
##                main      #10604         +/-   ##
===================================================
- Coverage   11.40490%   11.40001%   -0.00489%     
===================================================
  Files           3214        3214                 
  Lines         648458      648394         -64     
  Branches       71511       71511                 
===================================================
- Hits           73956       73917         -39     
+ Misses        573340      573309         -31     
- Partials        1162        1168          +6     

Flag	Coverage Δ
Debug	11.40001% <47.58621%> (-0.00489%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[lindexi]

I do not think this line be the good changes, 4 line to 5 line with two branch