Since Orekit developers like releasing without any Code Smells, I started working on fixing all code smells in develop branch. The list is quite long (i.e., 127 code smells) but most of them are identical and quick to fix.
The majority of code smells is like the following example
public List<Bunny> getWonderfulBunnies() { return bunnies; }
The code smells is clearly an immutability issue.
The fix is easy, we should just create a copy of the List. They are multiple solutions for that [1]. But, the most recommended solution is to create an unmodifiable copy of the List [2] [3] using List.copyOf(). However, it is an important behavior change and I would like to have your opinion on that.
Actually there are 1300 code smells on the develop branch right now. Most of them are in “old code” and were introduced by sonar when we upgraded to java 21. It has to do with getters on “mutable” objects or at least what sonar thinks is mutable. @Vincent might already be working on it?
Yeah i started looking at it in more details yesterday, most of them are not relevant but a fraction of them might need to be fixed as @bcazabonne suggests.
In this case we can either use a List.copyOf but we might also consider using a Collections.unmodifiableList which will provide a read-access only to the list (cheaper but the owner can still modify it and it will affect it). In the end it is a matter of what we want, performance vs immutability, which may not be the same for every case i guess.
This has always been a pain since years. The logic behind the check was that every object must be immutable, which is already an extremely strong assumption. I had a conversation with the guy at checkstyle doing that, and it led to nowhere, so I just gave up. Another of his assumptions was that if you have an add method, it means you mutate a container, but when dealing with fields in Hipparchus, add is not a mutation operation. Using the name of the method to guess what is done was also wrong IMHO, because you could write a mutation operation with a name that was not anticipated by checkstyle (say an addToSelf or thisMethodWithAWeirdNameMutatesTheInstanceWithoutCheckstyleKnowingItMuhahahaha), then it was not considered to change the object. I sincerely considered just shutting this check off.