Skip to content

[LANG-1426] Corrected usage examples in Javadocs#458

Merged
garydgregory merged 2 commits intoapache:masterfrom
mikkomaunu:LANG-1426-StringUtils-truncate-Javadoc
Sep 25, 2019
Merged

[LANG-1426] Corrected usage examples in Javadocs#458
garydgregory merged 2 commits intoapache:masterfrom
mikkomaunu:LANG-1426-StringUtils-truncate-Javadoc

Conversation

@mikkomaunu
Copy link
Copy Markdown
Contributor

Fixed documentation. No tests added because existings unit tests seam to cover these code examples reasonably well.

@coveralls
Copy link
Copy Markdown

coveralls commented Sep 15, 2019

Coverage Status

Coverage increased (+0.007%) to 95.21% when pulling 358c0d6 on mikkomaunu:LANG-1426-StringUtils-truncate-Javadoc into 29723e8 on apache:master.

@garydgregory
Copy link
Copy Markdown
Member

Why not also properly document legal inputs with @throws?

@mikkomaunu
Copy link
Copy Markdown
Contributor Author

Why not also properly document legal inputs with @throws?

Makes perfectly sense. Which approach is preferred:

One throws per Exception type:

@throws IllegalArgumentException If {@code offset} or {@code maxWidth} is less than {@code 0}

or:

@throws IllegalArgumentException If {@code offset} is less than {@code 0} 
@throws IllegalArgumentException If {@code maxWidth} is less than {@code 0}

@mikkomaunu
Copy link
Copy Markdown
Contributor Author

Related theme, should one also fix tests that assert exceptions? Currently message does not describe too well why test failed, instead it often simply repeats Exception message. Also that does not alway match.

For example. Following assertion uses "maxWith cannot be negative" as message. That does not describe why test failed. Also, because first argument is validated first, actual message in exception is "offset cannot be negative"


assertThrows(
                IllegalArgumentException.class,
                () -> StringUtils.truncate(null, Integer.MIN_VALUE, Integer.MIN_VALUE),
                "maxWith cannot be negative");

I see two approaches to fix this:

  1. improve message (less code, changing order of parameter validation does not affect tests):
        assertThrows(
                IllegalArgumentException.class,
                () -> StringUtils.truncate(null, Integer.MIN_VALUE, Integer.MIN_VALUE),
                "Expected IllegalArgumentException when both offset and and maxWidth are negative");
  1. improve message and assert also Exception message (significantly more code, does not play well when code does have bunch of unrelated assertions in one test method):
        final IllegalArgumentException exceptionWhenOffsetAndMaxWidthAreNegative = assertThrows(
                IllegalArgumentException.class,
                () -> StringUtils.truncate(null, Integer.MIN_VALUE, Integer.MIN_VALUE),
                "Expected IllegalArgumentException when both offset and and maxWidth are negative");
        assertEquals("offset cannot be negative", exceptionWhenOffsetAndMaxWidthAreNegative.getMessage(), "Wrong Exception message");

When exception is thrown because maxWidth is negative, then there is misspelling: "maxWith cannot be negative";

@garydgregory
Copy link
Copy Markdown
Member

WRT @throws: One.

Copy link
Copy Markdown
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing these! Looks good to me, and agree on the improvement suggested by @garydgregory

@garydgregory
Copy link
Copy Markdown
Member

I we should do what makes the code easier to understand from the perspective of maintenance: If a new person looks at the test, what is best for them to understand what is being tested. What would make it easier to add another test. And so on.

…ate and consistently mimicking exception message in assertion message
@kinow
Copy link
Copy Markdown
Member

kinow commented Sep 22, 2019

I think @mikkomaunu has updated the PR with your suggestions @garydgregory . Should be good to be reviewed now 🎉

@garydgregory
Copy link
Copy Markdown
Member

@garydgregory garydgregory merged commit 089b43a into apache:master Sep 25, 2019
asfgit pushed a commit that referenced this pull request Sep 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants