Stronger encapsulation of collection fields in MavenProject

[desruisseaux]

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 that the current collections are computed (e.g. so that if sourceDirectories is not specified, then it is built by default from the <Source> element). 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 delegate methods 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 the getDependencyArtifacts() method 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 pull request contains also the following changes. This changes are isolated in separated commits:

• 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 precious 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:

if (!getModel().getDelegate().getSubprojects().isEmpty()) {
    return getModel().getDelegate().getSubprojects();
}

Can be replaced by:

List<String> subprojects = getModel().getDelegate().getSubprojects();
if (!subprojects.isEmpty()) {
    return subprojects;
}

[cstamas]

We really need to reverse things: have maven-api-impl implement all these, and let "legacy" (like compat was in mvn3) rely on new API, instead the other way around...

[desruisseaux]

We really need to reverse things: have maven-api-impl implement all these, and let "legacy" (like compat was in mvn3) rely on new API, instead the other way around...

It is fine for me. The problem is just that I don't really know what is new API and what is legacy API. If MavenProject is legacy API, I didn't realized that. If this is the case, maybe before any further work, we should start by putting an @Deprecated annotation on all legacy API, together with a link to new API?

[cstamas]

Yes, anything in maven-xxx is legacy except api/ and of course maven-api-impl that implements API. Currently "new" implementation relies on legacy, but we should turn it around....