change code according to Clean Code rules

[mjechow]

I made some changes again. I think I finished this work now.

What one could do further:

• There are some tests missing asserts. We could add AssertJ and use AssertDoesNotThrow(()->) in this cases. But I don't think that's worth it.
• Sonar doesn't like it, when there are more than 5 parents. There are 6 or 7 from some classes.
• Some method are quite big and complex. Sonar suggests to simplify them.
• • public QualifyingProperty verify(
• • public void convertFromObjectTree(
• • void buildKeyInfo(
• • static KeyInfoRes process(
• • static ReferencesRes processReferences(
• the two ObjectFactory classes are really big, they should be split and
• the constructors of SignerXXX have more than 10 parameter, we could introduce a builder pattern for them
• sonar suggests in private static class AllCertificatesSelector implements Selector<X509CertificateHolder>: A copy constructor, copy factory or a custom copy function are suitable alternatives to the Object.clone
• Optional should not be used in parameters

[luisgoncalves]

Thanks for the cleanup!
I've added a couple of comments that I think should be addressed.

[mjechow]

Thanks for the comments. What do you think about the open tasks I mentioned. should I open some issues? I think the last three should be resolved, but there would be braking changes as well.

[luisgoncalves]

Thanks for the follow-up. I just noted one detail there in XmlQualifierType.

[luisgoncalves]

Regarding the other open tasks, here are my thoughts:

Some method are quite big and complex. Sonar suggests to simplify them.

Could be good to simplify a bit, but it's stable code, so really low prio and maybe not worth it in some cases. I'll have a look at the last ones in the list, as they could be broken into smaller methods for readability.

the two ObjectFactory classes are really big, they should be split and

It's auto-generated code. I'd say it shouldn't be changed, but instead ignored from analysis

the constructors of SignerXXX have more than 10 parameter, we could introduce a builder pattern for them

There are instantiated via DI, so no caller would benefit from the builder. If anything, this may illustrate that responsibilities could be "grouped" somehow to avoid so many dependencies. Not sure.

sonar suggests in private static class AllCertificatesSelector implements Selector: A copy constructor, copy factory or a custom copy function are suitable alternatives to the Object.clone

The clone method comes from the implemented interface, plus it's a private class. IF you find a way to get rid of the warning, feel free to fix.

Optional should not be used in parameters

This one comes from SignerT/SignerC, right? Those classes are package-private, so changes are not breaking. But they are instantiated by Guice (DI). I think Guice supports @Nullable, so the parameter could be nullable instead of Optional. feel free to fix.

[mjechow]

There is one more:

ExclusiveCanonicalXMLParamsMarshaller.doMarshal() should return Collections.emptyList(); not Null. But this is a breaking change as well.

[mjechow]

Unfortunately its not easily possible the change the Optional to @Nullable, because you have to bind a null explicitly. I don't know guice, so I don't resolve this issue.

[luisgoncalves]

I'll handle those other improvements. Thanks for the suggestions.

[mjechow]

Is there a chance of a new release containing this changes anytime soon?

[luisgoncalves]

Yeah, I'll push one out soon.

[mjechow]

Thanks for the release. I could already integrate it in my project. One remark for the release notes: it not "Guice to 6.0.0" but "Guice to 7.0.0".