coobird / thumbnailator

Thumbnailator - a thumbnail generation library for Java
MIT License
5.14k stars 784 forks source link

String comparison using '==', instead of 'equals()' #153

Closed osbald closed 4 years ago

osbald commented 4 years ago

While looking at the code in my IDE the linter plugin I use highlighted a number of lines in the code where direct comparisons == or != where being used to compare string instead of using Object.equals(). These are probally unintentional bugs.

I've attached a list of line numbers and what I think the fixes would look like:

net/coobird/thumbnailator/tasks/io/OutputStreamImageSink.java:107

        if (!ThumbnailParameter.DEFAULT_FORMAT_TYPE.equals(param.getOutputFormatType()))

net/coobird/thumbnailator/tasks/SourceSinkThumbnailTask.java:93

        if (ThumbnailParameter.ORIGINAL_FORMAT.equals(paramOutputFormat))

net/coobird/thumbnailator/util/ThumbnailatorUtils.java:55

        if (ThumbnailParameter.ORIGINAL_FORMAT.equals(format))      

net/coobird/thumbnailator/util/ThumbnailatorUtils.java:82

        if (ThumbnailParameter.ORIGINAL_FORMAT.equals(format))  

net/coobird/thumbnailator/util/ThumbnailatorUtils.java:129-130

        if (ThumbnailParameter.ORIGINAL_FORMAT.equals(format)
                && ThumbnailParameter.DEFAULT_FORMAT_TYPE.equals(type))

net/coobird/thumbnailator/util/ThumbnailatorUtils.java:134-135                      

        else if (ThumbnailParameter.ORIGINAL_FORMAT.equals(format)
                && !ThumbnailParameter.DEFAULT_FORMAT_TYPE.equals(type))

net/coobird/thumbnailator/util/ThumbnailatorUtils.java:139

        else if (ThumbnailParameter.DEFAULT_FORMAT_TYPE.equals(type))

net/coobird/thumbnailator/Thumbnails.java:1836

            if (!ThumbnailParameter.DEFAULT_FORMAT_TYPE.equals(formatType)      

Note: Also changed order of some of the comparisons above to avoid NPEs out of habit i.e. <constant>.equals(<variable>) so the variable can be null without the comparison causing an NPE.

Environment

osbald commented 4 years ago

Closing this as minor. I think the issue is more the Java 5 compatibility? which I wasn't expecting. So you're using Strings here like they are enums. So long as users only ever use the predefined Thumbnailator constants the existing code will work. It's only if they go calling these 'internal' classes above with their own Strings things might go awry.

Enums would help make the API more explicit but that's a different issue.

coobird commented 4 years ago

Just FYI, as bizarre as it may seem, the == are intentional; the constants you've pointed out are actually null values, so the comparison needs to be a == rather than equals which would actually lead to a NPE. (I definitely remember thinking to myself how bizarre the code looked when I was writing this.)