Skip to content

[MJAVADOC-825] Prefer NullPointerExceptions for null arguments #350

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
elharo merged 6 commits into from
Dec 8, 2024
Merged

[MJAVADOC-825] Prefer NullPointerExceptions for null arguments #350

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e2e2d32
Prefer NullPointerExceptions for null arguments
elharo Dec 5, 2024
d78c171
throws
elharo Dec 5, 2024
40ac53f
with
elharo Dec 5, 2024
3838624
of
elharo Dec 5, 2024
fdf3c41
on
elharo Dec 5, 2024
f22c7ad
of
elharo Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions src/main/java/org/apache/maven/plugins/javadoc/JavadocUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,11 +534,16 @@ protected static JavaVersion getJavadocVersion(File javadocExe)
* @return the version of the javadoc for the output, only digits and dots
* @throws PatternSyntaxException if the output doesn't match with the output pattern
* {@code (?s).*?[^a-zA-Z]([0-9]+\\.?[0-9]*)(\\.([0-9]+))?.*}.
* @throws IllegalArgumentException if the output is null
* @throws NullPointerException if the output is null
* @throws IllegalArgumentException if the output is empty
*/
protected static String extractJavadocVersion(String output) throws IllegalArgumentException {
Copy link
Contributor

@pzygielo pzygielo Dec 5, 2024

Choose a reason for hiding this comment

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

Given IAEx being declared as thrown by, perhaps NPEx could also be added for style consistency?
Or IAEx declaration removed?

Copy link
Contributor Author

@elharo elharo Dec 5, 2024

Choose a reason for hiding this comment

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

Both have @throws clauses. Or did you mean something else?

Copy link
Contributor

@pzygielo pzygielo Dec 5, 2024

Choose a reason for hiding this comment

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

IllegalArgumentException is RTEx (as NPEx is). And is declared as being thrown by this method. Thus - let's add NullPointerException to the throws list.
Or remove throws.
Here, and in the second modified method.

Copy link
Contributor Author

@elharo elharo Dec 5, 2024

Choose a reason for hiding this comment

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

But that's already done in this PR. Are you looking at the left side (old code) instead of the right side (new code)?

Copy link
Contributor

@pzygielo pzygielo Dec 5, 2024

Choose a reason for hiding this comment

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

I see this:

protected static String extractJavadocVersion(String output) throws IllegalArgumentException {

I'd like to see this instead:

protected static String extractJavadocVersion(String output) throws IllegalArgumentException, NullPointerException {

or this:

protected static String extractJavadocVersion(String output) {

Copy link
Contributor Author

@elharo elharo Dec 5, 2024

Choose a reason for hiding this comment

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

Good spot. That should go the other way per guidelines. Removed both.

if (output == null || output.isEmpty()) {
throw new IllegalArgumentException("The output could not be null.");
if (output == null) {
throw new NullPointerException("The output cannot be null.");
}

if (output.isEmpty()) {
throw new IllegalArgumentException("The output cannot be empty.");
}

Pattern pattern = EXTRACT_JAVADOC_VERSION_PATTERN;
Expand Down Expand Up @@ -591,14 +596,18 @@ protected static String extractJavadocVersion(String output) throws IllegalArgum
* </table>
*
* @param memory the memory to be parsed, not null.
* @return the memory parsed with a supported unit. If no unit specified in the <code>memory</code> parameter, the
* @return the memory parsed with a supported unit. If no unit is specified in the <code>memory</code> argument, the
* default unit is <code>m</code>. The units <code>g | gb</code> or <code>t | tb</code> will be converted in
* <code>m</code>.
* @throws IllegalArgumentException if the <code>memory</code> parameter is null or doesn't match any pattern.
* @throws NullPointerException if the <code>memory</code> argument is null
* @throws IllegalArgumentException if the <code>memory</code> argument doesn't match any pattern.
*/
protected static String parseJavadocMemory(String memory) throws IllegalArgumentException {
if (memory == null || memory.isEmpty()) {
throw new IllegalArgumentException("The memory could not be null.");
if (memory == null) {
throw new NullPointerException("The memory cannot be null.");
}
if (memory.isEmpty()) {
throw new IllegalArgumentException("The memory cannot be empty.");
}

Matcher m0 = PARSE_JAVADOC_MEMORY_PATTERN_0.matcher(memory);
Expand Down
69 changes: 39 additions & 30 deletions src/test/java/org/apache/maven/plugins/javadoc/JavadocUtilTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.regex.PatternSyntaxException;

import org.apache.maven.plugin.testing.stubs.MavenProjectStub;
import org.apache.maven.plugins.javadoc.ProxyServer.AuthAsyncProxyServlet;
Expand All @@ -61,21 +60,31 @@
* @author <a href="mailto:[email protected]">Vincent Siveton</a>
*/
public class JavadocUtilTest extends PlexusTestCase {
/**
* Method to test the javadoc version parsing.
*
*/
public void testParseJavadocVersion() {
String version = null;

public void testParseJavadocVersion_Null() {
try {
JavadocUtil.extractJavadocVersion(version);
JavadocUtil.extractJavadocVersion(null);
fail("Not catch null");
} catch (IllegalArgumentException e) {
assertTrue(true);
} catch (NullPointerException ex) {
assertNotNull(ex.getMessage());
}
}

public void testParseJavadocVersion_EmptyString() {
try {
JavadocUtil.extractJavadocVersion("");
fail("Not catch empty version");
} catch (IllegalArgumentException ex) {
assertNotNull(ex.getMessage());
}
}

/**
* Test the javadoc version parsing.
*/
public void testParseJavadocVersion() {
// Sun JDK 1.4
version = "java full version \"1.4.2_12-b03\"";
String version = "java full version \"1.4.2_12-b03\"";
assertEquals("1.4.2", JavadocUtil.extractJavadocVersion(version));

// Sun JDK 1.5
Expand Down Expand Up @@ -126,15 +135,6 @@ public void testParseJavadocVersion() {
version = "java full version \"1.4\"";
assertEquals("1.4", JavadocUtil.extractJavadocVersion(version));

version = "java full version \"1.A.B_07-164\"";
try {
JavadocUtil.extractJavadocVersion(version);
// does not fail since JEP 223 support addition
// assertTrue( "Not catch wrong pattern", false );
} catch (PatternSyntaxException e) {
assertTrue(true);
}

version = "SCO-UNIX-J2SE-1.5.0_09*FCS-UW714-OSR6*_20061114";
assertEquals("1.5.0", JavadocUtil.extractJavadocVersion(version));

Expand Down Expand Up @@ -163,20 +163,29 @@ public void testParseJavadocVersion() {
assertEquals("10.0.1", JavadocUtil.extractJavadocVersion(version));
}

/**
* Method to test the javadoc memory parsing.
*
*/
public void testParseJavadocMemory() {
String memory = null;
public void testParseJavadocMemory_null() {
try {
JavadocUtil.parseJavadocMemory(memory);
JavadocUtil.parseJavadocMemory(null);
fail("Not catch null");
} catch (IllegalArgumentException e) {
assertTrue(true);
} catch (NullPointerException ex) {
assertNotNull(ex.getMessage());
}
}

public void testParseJavadocMemory_empty() {
try {
JavadocUtil.parseJavadocMemory("");
fail("Not catch null");
} catch (IllegalArgumentException ex) {
assertNotNull(ex.getMessage());
}
}

memory = "128";
/**
* Method to test the javadoc memory parsing.
*/
public void testParseJavadocMemory() {
String memory = "128";
assertEquals(JavadocUtil.parseJavadocMemory(memory), "128m");

memory = "128k";
Expand Down
Loading