VIX-3831 Fixing Vixen.Application.csproj Warnings#798
VIX-3831 Fixing Vixen.Application.csproj Warnings#798johncbaur wants to merge 5 commits intoVixenLights:feature/VIX-3693from
Conversation
VIX-3830 Improve Prop Overlap Rubberband Selection
VIX-3830 Restore projection depth logic
VIX-3830 Improve Prop Overlap Rubberband Selection
VIX-3830 Restore projection depth logic
VIX-3831 Fixing Vixen.Application.csproj Warnings
jeffu231
left a comment
jeffu231
left a comment
There was a problem hiding this comment.
Glad to see this cleanup. The warnings are definite getting noisy in both streams of work.
| if (_setupPatchingSimple == null) | ||
| { | ||
| throw new Exception(nameof(_setupPatchingSimple) + " is null!"); | ||
| } |
There was a problem hiding this comment.
If we are cleaning these up, which I am all for, can we use the latest construct for them. It condenses 4 lines into one, supports refactoring and is very readable.
ArgumentNullException.ThrowIfNull(_setupPatchingSimple);
There was a problem hiding this comment.
@jeffu231 - I have no issue replacing the above code with the suggested ArgumentNullException but I am not sure we would be using it as intended. _setupPatchingSimple is not a method argument. I could see using InvalidOperationException. Do you still want to use ArgumentNullException here and for all member variable checks?
There was a problem hiding this comment.
If you look at the source code for that method, it does the same thing as the lines you are replacing. From what I have seen Microsoft uses it in many places aside from just method parameter validation, so in my opinion there is no reason not to use it. The end result is you want to be notified by exception of the field is null.
There was a problem hiding this comment.
As I think about it more, I suppose it would also be possible to create an static class that would accomplish similar logic and throw InvalidOperationException to wrap the code and make it more readable. Then you would have the preferred exception when it is not an argument.
Edit to specify class as an extension would not likely work.
| { | ||
| // Initialize the preview background | ||
| _background = new PropPreviewBackground("a469f44f-a21e-485e-af05-95b9b69a3fd0.jpg", 1.0f); | ||
| Background = new PropPreviewBackground("a469f44f-a21e-485e-af05-95b9b69a3fd0.jpg", 1.0f); |
There was a problem hiding this comment.
Is there an issue to resolve this hard coded background image. Does not have to be in the work queue yet, but just making sure it is captured.
There was a problem hiding this comment.
Multiple candidates for simpler ArgumentNull pattern.
There was a problem hiding this comment.
Multiple candidates for simpler ArgumentNull pattern.
2 participants
VIX-3831 Fixing Vixen.Application.csproj Warnings
Need to merge VIX-3830 before processing this pull request.