LANG-1720: Fix Javadoc description of exceptions thrown for Conversion class

[arthurscchan]

The Javadoc comments for most methods of the Conversion class contain detailed descriptions of the reason for some expected exceptions thrown. These exceptions include but are not limited to different IndexOutOfBoundsException or NullPointerException. It is found that all variants of the hexTo*() method will throw StringIndexOutOfBoundsException if srcPos + nHex > src.length but this is not documented. Thus this PR provides an improvement of the relative Javadoc, mentioning that these methods could throw StringIndexOutOfBoundsException.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (c52fec6) 92.14% compared to head (8043291) 92.15%. Report is 8 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1139      +/-   ##
============================================
+ Coverage     92.14%   92.15%   +0.01%     
- Complexity     7595     7596       +1     
============================================
  Files           200      200              
  Lines         15910    15910              
  Branches       2925     2925              
============================================
+ Hits          14660    14662       +2     
+ Misses          676      675       -1     
+ Partials        574      573       -1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[garydgregory]

@arthurscchan I'm not so sure about this one. Maybe the original runtime exception should be rethrown as an IllegalArgumentException.

[arthurscchan]

I am ok with either way. But it may need to change other methods to remain consistency. For example, the other method here, just throw IndexOutOfBoundsException directly and document that. @garydgregory

[elharo]

I agree with @garydgregory Throwing IndexOutOfBoundsExceptions from these methods is a bug. These should all be IllegalArgumentException. Filed https://issues.apache.org/jira/browse/LANG-1725

[michael-o]

I agree with @garydgregory Throwing IndexOutOfBoundsExceptions from these methods i a bug. These should all be IllegalArgumentException. Filed https://issues.apache.org/jira/browse/LANG-1725

Agree as well.

[elharo]

Given that we do want to change this behavior, it's probably not a good idea to Javadoc the current behavior, so I suggest closing this PR. However, it is still very valuable for calling our attention to this issue. Thanks.

[garydgregory]

I agree this PR should be closed as is. I'll leave it open for now if @arthurscchan wants to update it with code changes instead.

[elharo]

On further investigation, I'm not so sure. The API here is really strange. Since the indexes that can be out of bounds are passed in from client code, StringIndexOutOfBoundsException seems appropriate in at least some cases.

[garydgregory]

@elharo and all Taking the same approach as java.util.Arrays seems reasonable but that class checks and throws exceptions (IAE and ArrayIndexOutOfBoundsException) explicitly instead of letting code fail. Thoughts?

[elharo]

Letting code fail is fine and avoids extra work on the happy path. If an explicit check isn't needed, it isn't needed. The important thing is to document and test the desired behavior. As long as the behavior is tested, how the behavior is implemented is a detail.

[garydgregory]

It sounds like we are ok with this PR then. @elharo @michael-o ?

[garydgregory]

Hi all, @arthurscchan , @elharo

I've reviewed this PR, the code, and pushed to git master with better input checks:

• Conversion.hexTo*() methods now throw IllegalArgumentException instead of StringIndexOutOfBoundsException
• The Javadoc is updated but lets the exception messages give the new details
• The exception messages are better
• The rest of the class will get similar updates

[garydgregory]

Closing.

[garydgregory]

After reworking the class locally to throw IEA instead of [Array|String]IndexOutOfBoundsException, it looks like we work too hard for the happy path when some of the APIs already document [Array|String]IndexOutOfBoundsException. Since this is a low-level API, it also makes more sense to keep closer to what the JRE does. We also have our Validate class which throws [Array|String]IndexOutOfBoundsException when validating array bounds. So I'll revert to the released style and update the Javadoc to be consistent.