walkmod/walkmod-pmd-plugin

merging if conditions looses comment

cal101 opened this issue · 2 comments

-        if (imageFile.exists()) {
-            // lock file for some time
-            if (hotlist.putFile(imageFile)) {
-                if (log.isDebugEnabled()) {
-                    log.debug("getSourceImage, HIT, imageId: " + imageId + ", file: " + imageFile);
-                }
-                return imageFile;
+        if (imageFile.exists() && hotlist.putFile(imageFile)) {
+            if (log.isDebugEnabled()) {
+                log.debug("getSourceImage, HIT, imageId: " + imageId + ", file: " + imageFile);
             }
+            return imageFile;
rpau commented

Well, I do not find an easy solution for this, because when WalkMod compares the nodes of the original AST vs the updated AST to define the replacements, additions or removals; WalkMod cannot discover where to print a comment.

For another hand, when a developer design a visitor, do not know the place where a new node will be written (this is the nice part), so comments are loosed.

What do you think?

In our code multiple places were refactored that follwed the style

if (some-more-global-condition) {
   // some long comment describing what is done in this code block
   // ...
   // ...
  <some code block> 
}

if happens to be only a simple if the refactoring took place.

Since the kind of refactorings done using this plugin need a review of the refactored code before committing where unneeded information can be possibly removed I would prefer something as follows.

Let comments of removed statements "bubble up" to the next statement and mark the comments as such.

if (a)
   // comment
   if (b)

becomes

// refactored-by-<plugin>: begin
// comment
// refactored-by<plugin>: end
if (a && b)

So nothing is lost and one get noticed that the target of the comment may be wrong now.
Or you could refactor it to:

if (a
   // comment 
   && b)