desruisseaux opened a new pull request, #1744:
URL: https://github.com/apache/maven/pull/1744
Stronger encapsulation of collection fields with immutability and defensive
copies. Clarification of the expectations about which fields or return values
can be null.
# Goals
The goal is to make easier to add new collections in a future commit (for a
list of `<Source>` elements), or to modify the way the current collections are
computed (e.g. so that if `sourceDirectories` is no specified, it is built by
default from the `<Source>`). This is currently difficult to do if we are
unsure about the contracts of existing collections.
# Main changes
The following rules are applied in this commit (the previous situation was a
mix of different practices):
* All getter methods except `getDependencyArtifacts()` return an empty
collection instead of null.
* All setter methods except deelgates make a defensive copy of the given
collection, preserving order.
* All getter methods returns an unmodifiable (but not necessarily immutable)
collection.
* The collections of properties that do not have an `addFoo(…)` method are
immutable.
* The `clone()` method copies all non-immutable collections, and only them.
* `MavenProject(MavenProject)` assigns fields directly without invoking
setter methods,
for avoiding the "this-escaped" compiler warning.
An exception is made for `getDependencyArtifacts()` for compatibility
reason, because we observed that Maven codes in other classes test for the
nullity of that property instead of emptiness.
# Opportunistic changes
This commit contains also the following changes:
* Clarified whether a field, parameter or return value is expected to be
null.
* Added `@Nullable`, `@Nonnull` and `@SuppressWarnings` annotations, and
some null checks.
* Added some Javadoc telling whether null is accepted, and whether the
collection is copied / immutable.
* Removed some empty lines, not because they are bad but because Checkstyle
imposes a limit of 2000 lines.
* Reduced some code duplication, e.g. with the addition of a private
`toDependency(Artifact)` method.
* Rewrite some methods using Stream because it saves some of those previous
lines limited by Checkstyle.
* Renamed some parameters or local variables for avoiding to hide a field,
except when it should be the same thing.
* Avoid invoking methods many times when the value should not change.
For example, the following code:
```java
if (!getModel().getDelegate().getSubprojects().isEmpty()) {
return getModel().getDelegate().getSubprojects();
}
```
Can be replaced by:
```java
List<String> subprojects = getModel().getDelegate().getSubprojects();
if (!subprojects.isEmpty()) {
return subprojects;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
