Throw exception in ViewportAdapter constructor if the GraphicsDevice is null #838
Conversation
|
Firstly, thank you for attention! It's a good idea, but why do you use the custom exception? There is People often use this way, so more easy to understand. |
There was a problem hiding this comment.
Thank you for the PR @safoster88
Please make the requested changes and then it can be merged.
| public abstract class ViewportAdapter : IDisposable | ||
| { | ||
| protected ViewportAdapter(GraphicsDevice graphicsDevice) | ||
| { | ||
| if (graphicsDevice is null) | ||
| { | ||
| throw new NullGraphicsDeviceException($"Attempted to construct {nameof(ViewportAdapter)} but {nameof(GraphicsDevice)} is null"); |
There was a problem hiding this comment.
This should just throw an ArgumentNullException since the issue here is that the argument is null, not that the graphics device itself is null.
Please use the new method for checking
ArgumentNullException.ThrowIfNull(graphicsDevice);| { | ||
| var act = () => new TestViewportAdapter(null); | ||
|
|
||
| Assert.Throws<NullGraphicsDeviceException>(act); |
There was a problem hiding this comment.
After making the suggested change of using ArgumentNullException this should be updated to check for that exception type.
There was a problem hiding this comment.
This custom exception is not needed. The problem being solved is that the graphics device argument for the method is null, not that the GraphicsDevice itself is null. This can be removed and the exception changed to use ArgumentNullException to better communicate to consumers that the argument being null is the issue.
|
Is this resolved? I'd like to take a shot at my first PR if it's not! |
If the viewport adapter is initialized in the constructor of game, GraphicsDevice will be null.
There is no check for this, so the null ref only occurs when trying to call GetViewMatrix().
This PR aims to improve the public API.