lesquoyb
commented
3 weeks ago Ok so I've read your remaining spotbugs violations, and I made a more human readable summary with my point of view on how seriously you should take them:
- the 3 CT_CONSTRUCTOR_THROW it's up to you to fix it or not but I don't think it's important, if an exception is raised in those constructors gama will probably catch it and display it the user so there's no real risk even though it's a bit ugly
- the 6 WMI_WRONG_MAP_ITERATOR are just telling you that you could use
entrySetto get the key and value in the same object for your loops instead of iterating onkeySetand get the value with thegetfunction. To me it doesn't matter if you fix it or not as it's not in parts of the code where optimization could be important. - the 2 RV_RETURN_VALUE_IGNORED_BAD_PRACTICE are more important as they highlight that you didn't handle the case of error while creating a directory or deleting a file. I think you should probably use the
GAMA.reportAndThrowIfNeededmethod to stop the execution and report an error. - the SBSC_USE_STRINGBUFFER_CONCATENATION indicates that you could have used the
StringBuffer'sappendmethod instead of concatenated strings, which is more efficient, and particularly in a loop it could amount to good performance gains. I advise that you do it. - the 3 EI_EXPOSE_REP2 you could probably ignore. It indicates that you are keeping a reference on lists/maps that could be modified from the outside. If you want to be "clean" you could probably clone those lists/maps
- the NP_ALWAYS_NULL is very important, it indicates that you are trying to access a method on an object that we already know is null, thus it will raise a
NullPointerException. Reading the code it seems like the condition is wrong (&&instead of||) but as you told me you never have any exception you should consider completely deleting this constructor as it's probably never called - the NP_NULL_PARAM_DEREF is raised because it is (theoretically) possible to reach this code with a value of
nullfor the variablefrwhich will lead to more exception. In reality it may not be reached because some exceptions are raised before, but I'm not sure. I recommend that either you explicitly use a return in the catch blocks to make sure that this code is never reach in case of exception, or you move all the code following the initialization offrin thetryblock. - the OS_OPEN_STREAM is raised because you open a stream but it is never explicitly closed in case of exception. I think you should also solve this one and this can be done using a try with resource. It would be something like:
try (BufferedReader br = new BufferedReader(fr)){.... This construction will make sure that br is disposed (and thus closed) at the end of the block. - And finally there are 3 RV_EXCEPTION_NOT_THROWN and I think those are really important as it means you have created exceptions that are not thrown. I've also recently learned about this, but
GamaRuntimeException.erroronly creates aGamaRuntimeExceptionobject, and you still need to raise the exception for it to be seen by the user. You can do so either withthrow GamaRuntimeException.create...or using the methodGAMA.reportAndThrowIfNeeded. I personally use theGAMA.reportAndThrowIfNeededsyntax as I think it's the mostgamaway but I'm not 100% sure it's the best option every time.
To be honest I could have resolved almost all of those by myself and commited the fixes, but I think it's important to do that kind of explanation as you will probably encounter them again and I might not always be available to fix them.
Sensitivity analysis methods, have been added to Gama as of 1.9.x, but have never been polished since, notably in the provided feedbacks and outputs. I list the foreseen improvement
There is also part of the improvement to further deploy SA features to every exploration methods:
outputsandresultsfacets. See #147Don't hesitate to add more task to upgrade SA & exploration in Gama.