Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

merging if conditions looses comment #7

Open
cal101 opened this issue Mar 7, 2017 · 2 comments
Open

merging if conditions looses comment #7

cal101 opened this issue Mar 7, 2017 · 2 comments

Comments

@cal101
Copy link
Contributor

cal101 commented Mar 7, 2017

-        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
Copy link
Contributor

rpau commented Mar 8, 2017

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
Copy link
Contributor Author

cal101 commented Mar 8, 2017

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants