Skip to content
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

Use ThrowHelper methods in more places #6184

Merged
merged 8 commits into from Mar 8, 2024
Merged

Conversation

turbedi
Copy link
Contributor

@turbedi turbedi commented Feb 18, 2024

ObjectDisposedException.ThrowIf(), ArgumentException.ThrowIfNullOrEmpty() and the ArgumentOutOfRangeException methods have been introduced in .NET 7/8

Copy link
Member

@Susko3 Susko3 left a comment

Choose a reason for hiding this comment

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

Seems ok but some error messages have changed for the worse.

Comment on lines -426 to +424
if (minSize > maxSize)
throw new ArgumentOutOfRangeException(nameof(minSize), $"Must be less than {nameof(maxSize)}.");
ArgumentOutOfRangeException.ThrowIfNegative(minSize);
ArgumentOutOfRangeException.ThrowIfGreaterThan(minSize, maxSize);
Copy link
Member

Choose a reason for hiding this comment

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

Just noting that maxSize no will longer appear in the error message.

master:

System.ArgumentOutOfRangeException : Must be less than maxSize. (Parameter 'minSize')

This PR:

System.ArgumentOutOfRangeException : minSize ('20') must be less than or equal to '10'. (Parameter 'minSize')
Actual value was 20.

osu.Framework/Testing/Drawables/TestGroupButton.cs Outdated Show resolved Hide resolved
osu.Framework/Utils/IncrementalBSplineBuilder.cs Outdated Show resolved Hide resolved
Comment on lines -64 to +63
if (minValue > maxValue)
throw new ArgumentOutOfRangeException(nameof(minValue), "The given minimum value must be less than or equal to the given maximum value.");
ArgumentOutOfRangeException.ThrowIfGreaterThan(minValue, maxValue);
Copy link
Member

Choose a reason for hiding this comment

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

See other comment about maxValue/maxSize no longer appearing in the error message. Same applies for the other usage of ThrowIfGreaterThan() in this file.

osu.Framework/Utils/IncrementalBSplineBuilder.cs Outdated Show resolved Hide resolved
@turbedi
Copy link
Contributor Author

turbedi commented Mar 1, 2024

I've addressed some of the comments. Should I also revert all usages of ThrowIfGreaterThan() to retain the max value in the error message, or is it fine like that?

@frenzibyte
Copy link
Member

Related: #3470

@smoogipoo
Copy link
Contributor

The other path I can suggest is to make an ExceptionUtils.ValidateRange(). Otherwise, I'm weakly okay with the change in messaging but I'd want other reviews.

@frenzibyte
Copy link
Member

I personally don't feel the exception message matters that much. Sure it might be a little bit cryptic, but the "must be greater than"/"must be lower than" is already giving it out I think.

@bdach
Copy link
Collaborator

bdach commented Mar 7, 2024

My view on the ThrowIfGreaterThan() thing is that I'd be fine with the max value not being printed only if it's a constant. So in cases where the max value isn't statically known just by looking at the direct call site it should print the max in the exception message.

Which this does, so I'm fine with this. The fact that it doesn't print the name of the other value feels whatever.

@turbedi
Copy link
Contributor Author

turbedi commented Mar 7, 2024

All comments have been addressed.

@bdach bdach merged commit fa05191 into ppy:master Mar 8, 2024
21 checks passed
@turbedi turbedi deleted the new_throwhelper branch March 8, 2024 09:01
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.

None yet

6 participants