Repository navigation
ImageFormatManager.TryFindFormatByFileExtension throws when null or whitespace is bad practice #2385
Description
Prerequisites
- I have written a descriptive issue title
- I have verified that I am running the latest version of ImageSharp
- I have verified if the problem exist in both
DEBUGandRELEASEmode - I have searched open and closed issues to ensure it has not already been reported
ImageSharp version
3.0.0
Other ImageSharp packages and versions
n/a
Environment (Operating system, version and so on)
Windows 10 & 11, 64bit, build 22000.1574
.NET Framework version
.NET 7
Description
The "ImageFormatManager.TryFindFormatByFileExtension" method here throws, which is against the TryX pattern.
public bool TryFindFormatByFileExtension(string extension, [NotNullWhen(true)] out IImageFormat? format)
{
Guard.NotNullOrWhiteSpace(extension, nameof(extension));
if (extension[0] == '.')
{
extension = extension[1..];
}
format = this.imageFormats.FirstOrDefault(x =>
x.FileExtensions.Contains(extension, StringComparer.OrdinalIgnoreCase));
return format is not null;
}
What you'd expect is that methods beginning with TryXXXX would never throw and instead simply return false.
Crude example but;
public bool TryFindFormatByFileExtension(string extension, [NotNullWhen(true)] out IImageFormat? format)
{
try
{
Guard.NotNullOrWhiteSpace(extension, nameof(extension));
if (extension[0] == '.')
{
extension = extension[1..];
}
format = this.imageFormats.FirstOrDefault(x =>
x.FileExtensions.Contains(extension, StringComparer.OrdinalIgnoreCase));
catch (Exception)
{
// Do nothing
}
return format is not null;
}
It should be the responsibility of the consuming application to ensure that it has thoroughly tested its implementation of this method.
Steps to Reproduce
// this
ImageFormatManager.TryFindFormatByFileExtension("", out ImageFormat? foo);
// or this
ImageFormatManager.TryFindFormatByFileExtension(null, out ImageFormat? foo);
Images
No response
Reacted by James Jackson-South
Activity
JimBobSquarePants commentedon Mar 6, 2023
Yep. That guard should be removed, and null handled by assigning format to null and returning false. I wouldn't use a try/catch there.
Feel free to open a PR if you have the time.
bjkippax commentedon Mar 6, 2023
@JimBobSquarePants I appreciate your swift response James. The try-catch was a crude example, I would never had intended to suggest that as a real-world fix/suggestion, just to illustrate my point.
One of my colleagues will be submitting a PR for this.
Best,
Ben
JimBobSquarePants commentedon Mar 6, 2023
Legend! Thanks
Ollie-Ave commentedon Mar 6, 2023
Hi, @bjkippax 's colleague here, I have created a PR for this change :). Here's the link:
#2386
Merge pull request #2386 from Ollie-Ave/main
JimBobSquarePants commentedon Apr 12, 2023
Fixed via #2386