Inconsistent method naming in LicenseCompareHelper
Open
Nobody has claimed this yet.
matching
- Dominant language
- Java
- Stars
- 71
- Forks
- 44
- Avg merge
- 12h 54m
- Merged PRs (30d)
- 7
Description
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 #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:
- Whether the method operates on licenses, or exceptions
- Whether the method performs exact matching of the text, or "finding" within the text
- 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)
- Whether the method returns the first match, or all matches
- 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.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the org.spdx.utility.compare.LicenseCompareHelper class and its Javadoc, then inventory the existing matching and diff method signatures against the proposed matrix. Review how the current methods are used before defining consistent names, compatibility delegations, and deprecations. Done means the naming scheme is implemented without removing old methods before a major release.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- backend
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100