Fixing bugs reported by Sonar

[danglotb]

Sonar gives us a list of "Bugs"

• [x] Do something with the "boolean" value returned by "delete".
• [x] Use try-with-resources or close this "Git" in a "finally" clause.
• [x] Call "Optional#isPresent()" or "!Optional#isEmpty()" before accessing the value.
• [x] Use try-with-resources or close this "FileReader" in a "finally" clause.

[dendritett]

I'd like to try this. Please assign this issue to me.

If you'll assign me, I have two questions. The first question. I have read the CONTRIBUTING.md, should I create a pull request for each of these 4 modifications? The second question is ・Do something with the "boolean" value returned by "delete". To solve this, what should we do if outputFd.delete() returns false? I think if delete() fails, then mkdir() will also fail.

I'm not used to contributing. And I'm not very good at English, so I'm sorry if the text doesn't make sense to you.

[danglotb]

Hello @dendritett

Welcome to Diff-JJoules! We are glad to have you here. :tada:

I would prefer you do one pull request per task, so 4 different pull requests in order to separate the concerns :smile:

I think that the idea of the first task is to check if the .delete() succeed or not, if not, I agree that we will throw an Exception in order to stop Diff-JJoules.

I suggest using a RuntimeException in order to avoid having to declare the exception in the constructor/method.

IHMO, the message should be simple, something like:

Something went wrong when trying to delete the folder <path to the folder>, please check your configuration

Thank you very much! Do not hesitate to ask for more details if you have any doubt. We can also discuss on the pull-request when you open it.

[dendritett]

Thank you for your kindness. 😄 I'm going to try this.

[dendritett]

I have one question. I'm planning to handle the remaining bugs reported as well, what should I do if there is no value of Optional?

Currently I think it throws a NoSuchElementException, should I have it throw a RuntimeException as well? Or should I not let it throw an exception and let it continue processing? For example

final CtAnnotation<? extends Annotation> testAnnotation =
    ctMethod.getAnnotations()
        .stream()
        .filter(ctAnnotation -> ctAnnotation.getType().getQualifiedName().endsWith("Test"))
        .findAny()
        .orElse();
if(testAnnotation != null){
    testAnnotation.replace(factory.createAnnotation(reference));
}
super.instrumentedTypes.add(declaringType);

[danglotb]

Hello,

The fact is that I think that the then branch of this if will be dead code because we have the following implementation of the method isToBeProcessed():

@Override
public boolean isToBeProcessed(CtMethod<?> candidate) {
    return super.isToBeProcessed(candidate) && candidate.getAnnotations()
        .stream()
        .anyMatch(ctAnnotation -> ctAnnotation.getType().getQualifiedName().endsWith("Test"));
}

Therefore, if noneMatch(), i.e. the opposite of findAny(), then the method process() won't be called. Thats the limit of the static analysis of Sonar.

However, we might be good student and apply your solution or use the method ifPresent() from the Stream API of Java:

ctMethod.getAnnotations()
        .stream()
        .filter(ctAnnotation -> ctAnnotation.getType().getQualifiedName().endsWith("Test"))
        .findAny()
        .ifPresent(ctAnnotation -> ctAnnotation.replace(factory.createAnnotation(reference)));

As you want.

[dendritett]

Thank you for explaining so politely and thoroughly. The method of using ifPresent() seems to maintain coverage without the need for additional test code. I'll try it that way.

[danglotb]

You completed the four tasks, great job!

[dendritett]

Thank you kindly for all your help!

I'm glad my first contribution was to you.

I had only ever written sources in Japan. So I am happy to be able to interact with people from all over the world in this way.:smile: