Inconsistent method naming in LicenseCompareHelper

[pmonks]

Problem Statement

I've found that the names of the various different permutations of the matching methods in the org.spdx.utility.compare.LicenseCompareHelper class are pretty confusing, and wonder if a more consistent naming scheme would be valuable?

This is partly fueled by:

• There is no overall pattern to how the various methods are named, making "at a glance inference of method variations" difficult.
• There are some missing permutations (e.g. see Missing method in LicenseCompareHelper: matchingStandardLicenseExceptionIds #391).
• There are inconsistencies in how exceptions are referred to (some methods use "LicenseException", while others use "Exception").
• There's conflation of "pure" matching methods (i.e. methods that answer the question "did this text contains one or more licenses / exceptions, and (optionally) if so, which ones?") with the methods that calculate diffs (i.e. methods that answer the question "what, if any, differences exist between this text and a given license / exception?").

It seems to me that the methods in this class fall into a sparse matrix defined by these criteria:

1. Whether the method operates on licenses, or exceptions
2. Whether the method performs exact matching of the text, or "finding" within the text
3. Whether the method considers only one listed license / exception, or multiple listed licenses / exceptions (and in the latter case, whether that's all listed licenses / exceptions, or a subset provided by the caller)
4. Whether the method returns the first match, or all matches
5. Whether the method is concerned with "pure" matching, or calculating differences

Coming up with a systematic naming scheme informed by these criteria has substantial value in helping to communicate how the LicenseCompareHelper class can be used for different matching / diffing scenarios.

A Proposed Naming Scheme

Methods that perform exact matching of the entire text:

Method Signature	Notes
boolean isLicense(ListedLicense lic, String text)
boolean isException(ListedLicenseException exc, String text)
String isOneOfTheseLicenses(List<ListedLicense> lics, String text)	Returns the id of the first listed license in lics that matched, or null if no match was found
String isOneOfTheseExceptions(List<ListedLicenseException> excs, String text)	Returns the id of the first listed exception in excs that matched, or null if no match was found
String isAnyLicense(String text)	Returns the id of the first listed license that matched, or null if no match was found
String isAnyException(String text)	Returns the id of the first listed exception that matched, or null if no match was found
List<String> listAllLicensesMatched(List<ListedLicense> lics, String text)	Returns the ids of all of the listed licenses in lics that matched, or null if no match was found. Might not be necessary, as this is a corner case for overlapping matching templates (e.g. GPL).
List<String> listAllExceptionsMatched(List<ListedLicenseException> excs, String text)	Returns the ids of all of the listed exceptions in excs that matched, or null if no match was found. Might not be necessary, as this is a corner case for overlapping matching templates (e.g. GPL).
List<String> listAllLicensesMatched(String text)	Returns the ids of all of the listed licenses that matched, or null if no match was found. Might not be necessary, as this is a corner case for overlapping matching templates (e.g. GPL).
List<String> listAllExceptionsMatched(String text)	Returns the ids of all of the listed exceptions that matched, or null if no match was found. Might not be necessary, as this is a corner case for overlapping matching templates (e.g. GPL).

Methods that perform finds within the text, but only return the first match:

Method Signature	Notes
boolean containsLicense(ListedLicense lic, String text)
boolean containsException(ListedLicenseException exc, String text)
String containsOneOfTheseLicenses(List<ListedLicense> lics, String text)	Returns the id of the first listed license in lics that matched, or null if no match was found
String containsOneOfTheseExceptions(List<ListedLicenseException> excs, String text)	Returns the id of the first listed exception in excs that matched, or null if no match was found
String containsAnyLicense(String text)	Returns the id of the first listed license that matched, or null if no match was found
String containsAnyException(String text)	Returns the id of the first listed exception that matched, or null if no match was found

Methods that perform finds within the text, and return all matches:

Method Signature	Notes
List<String> listAllLicensesFound(List<ListedLicense> lics, String text)	Returns the ids of all of the listed licenses in lics that matched, or null if no match was found
List<String> listAllExceptionsFound(List<ListedLicenseException> excs, String text)	Returns the ids of all of the listed exceptions in excs that matched, or null if no match was found
List<String> listAllLicensesFound(String text)	Returns the ids of all of the listed licenses that matched, or null if no match was found
List<String> listAllExceptionsFound(String text)	Returns the ids of all of the listed exceptions that matched, or null if no match was found

Methods that calculate diffs:

Method Signature	Notes
CompareTemplateOutputHandler.DifferenceDescription diffLicense(ListedLicense lic, String text)
CompareTemplateOutputHandler.DifferenceDescription diffException(ListedLicenseException exc, String text)

Notes:

• Many of these methods will share a common implementation - in fact in many cases they can be implemented as simple delegation calls to other methods (sometimes in a loop).
• Obviously the old method names can't be removed until the next major release of the library, but they could be re-implemented to simply delegate to the new methods, and marked as deprecated before then.
• I don't believe any of the method signatures listed above conflict with existing method signatures, but haven't done an exhaustive check.

[goneall]

Couple thoughts:

• In general, I support renaming the methods. Some of the existing methods reference "standard" licenses when "listed" licenses should be used. I like the proposed consistency in naming. A long time ago, some of us referred to the license list as "standard" licenses which many folks objected to, so we now use the term "listed" licenses.
• I don't support separating out the diff methods from the matching methods. Since the matching code collects diffs without any additional compute cost (at least today), I would prefer to return the full diff information which is easily queried to find out if it matches. If they were separate, it would lead to people calling the match then immediately calling the diff if it didn't match which would cost 2X performance.
• We should qualify that the licenses being compared are listed licenses - there are other methods that support comparing any 2 license texts which may be confusing if we don't include the "listed" qualifier in the name.

[pmonks]

@goneall wrote:

In general, I support renaming the methods. Some of the existing methods reference "standard" licenses when "listed" licenses should be used. I like the proposed consistency in naming. A long time ago, some of us referred to the license list as "standard" licenses which many folks objected to, so we now use the term "listed" licenses.

Oh yeah good catch - I hadn't noticed that inconsistency.

I don't support separating out the diff methods from the matching methods. Since the matching code collects diffs without any additional compute cost (at least today), I would prefer to return the full diff information which is easily queried to find out if it matches. If they were separate, it would lead to people calling the match then immediately calling the diff if it didn't match which would cost 2X performance.

I think that risk is better handled in the JavaDocs for those methods. I find dealing with CompareTemplateOutputHandler.DifferenceDescription objects when all I'm interested in is whether a license or exception was matched / found or not an additional (and avoidable) cognitive burden (especially since, in some cases, the answer I want is the negation of what those objects contain). Of course there's no reason that the methods that return a simple boolean couldn't simply delegate to the "diff" versions, then interrogate the CompareTemplateOutputHandler.DifferenceDescription object, return the boolean, and drop the rest of that object.

There may be some performance benefits to not constructing the CompareTemplateOutputHandler.DifferenceDescription object in the first place, from both a computation and memory use perspective, though I'm totally speculating here (and my memory is that those objects aren't particularly expensive to construct, or large once constructed).

We should qualify that the licenses being compared are listed licenses - there are other methods that support comparing any 2 license texts which may be confusing if we don't include the "listed" qualifier in the name.

Yeah agreed. This proposal was mostly focused on trying to list out all of the permutations of matching / finding methods, rather than trying to define a fully-thought-through naming convention.

In fact it also occurred to me that perhaps some of the complexity of all these permutations could be resolved by also adding an ExceptionCompareHelper class that largely mirrors LicenseCompareHelper. That partitions the number of method combinations in half, which might aid comprehension.

[goneall]

@pmonks I was going to do a quick fix for #391 but follow a future naming scheme as suggested here.

I was only going to implement 2 methods:

• List<String> listAllListedLicenseIdsMatched(String text) and deprecate the existing method String[] matchingStandardLicenseIds(String licenseText)
• List<String> listAllListedExceptionIdsMatched(String text)

Note I made two changes to the naming convention:

• Added Listed to the name to make it clear this will not match LicenseRef style licenses
• Added Ids to clarify the return type is the ID of the license or exception - even though it should be obvious from the return type, I think having it in the name makes it clearer

Let me know if you agree and I'll create a pull request with just the 2 changes.

We can clean up the other names in a separate pull request - (I'm working on a quick release to help a downstream project solve an issue with another library).

[pmonks]

@goneall yep that sounds good!