Fix redundant code in DefaultPluginXmlFactory:write

[Pankraz76]

Fix redundant code in DefaultPluginXmlFactory:write - avoiding DRY increasing cohesion lowering coupling

• Fix unused stream in DefaultPluginXmlFactory#write #2303

fix unreachable code:

maven/impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultModelXmlFactory.java

Line 103 in 6be7a12

return xml.read(is, request.isStrict(), source);

imho, this branch can never happen:

because of early exit check before

This is too defensive over here. The last if-else is the actual final possible condition and therefore should just return:

Its double-trouble anyway and therefore DRY violation - its a smell in eyes. 🎃

[Pankraz76]

So no, this is not a dead branch - it serves an important purpose in the API's flexibility.

Conclusion:
✅ The else block is reachable and necessary.
It serves as a fallback when only a URL is provided in the XmlReaderRequest.

Would you like help adding a unit test to validate this branch?

[Pankraz76]

But it's redundant then, as the behavior is now the same, and the end result is just without being overly complex and duplicating it—making the default exception explicit.

When the default behavior is the same in any case, it’s kind of simpler and should turn out equal in the end, right? So, classic redundancy—time for TDD (aka refactoring).

[elharo]

we shouldn't be catching raw exceptions here; probably fix that first

[Pankraz76]

IOException | XMLStreamException e

need this:
https://checkstyle.sourceforge.io/checks/coding/illegalcatch.html

[Pankraz76]

Of course, it's better API design to be precise and not broad—especially in this case.

If possible we should ensure this standard as well. In checkstyle its the same (of course as all the checkers are on), so i know this quite well. Its a good thing to strengthen the design.

apache/maven-shared-resources#73

[elharo]

similar

[Pankraz76]

#2312