walkmod / walkmod-pmd-plugin

Walkmod plugin to fix the code according the PMD rules
2 stars 2 forks source link

merging if conditions looses comment #7

Open cal101 opened 7 years ago

cal101 commented 7 years ago
-        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 7 years ago

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?

cal101 commented 7 years ago

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)