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

ImageFormatManager.TryFindFormatByFileExtension throws when null or whitespace is bad practice #2385

Closed
4 tasks done
bjkippax opened this issue Mar 6, 2023 · 5 comments
Closed
4 tasks done

Comments

@bjkippax
Copy link

bjkippax commented Mar 6, 2023

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 DEBUG and RELEASE mode
  • 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

@JimBobSquarePants
Copy link
Member

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
Copy link
Author

bjkippax commented 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
Copy link
Member

Legend! Thanks

@Ollie-Ave
Copy link
Contributor

Hi, @bjkippax 's colleague here, I have created a PR for this change :). Here's the link:

#2386

JimBobSquarePants added a commit that referenced this issue Mar 6, 2023
ISSUE #2385 - Implementing try get pattern to TryFindFormatByFileExtension(string extension, [NotNullWhen(true)] out IImageFormat? format)
@JimBobSquarePants
Copy link
Member

Fixed via #2386

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants