my first refactoring session #361

ghost · 2018-01-10T20:37:52Z

be warned though, several days ago an account named skylot appeared out of nowhere on my profile stars;
I contacted support, but they couldn't tell me anything but to change my password;
here, have a look: https://github.com/skylot
someone, who has a decompiler as project; oh my god, that's far beyond my skill level

tried to attend to the highlighted items, guided by the IDE

ghost · 2018-01-11T20:09:02Z

refactoring.api/src/org/netbeans/modules/refactoring/api/RefactoringSession.java

-                for (DataObject dob:DataObject.getRegistry().getModified()) {
-                    SaveCookie cookie = dob.getCookie(SaveCookie.class);
+                for (DataObject dataObject : DataObject.getRegistry().getModified()) {
+                    SaveCookie cookie = dataObject.getLookup().lookup(SaveCookie.class);


It appears there was a change here:

Before: dataObject.getCookie(SaveCookie.class)

After: dataObject.getLookup().lookup(SaveCookie.class)

getLookup().lookup(SaveCookie.class) is the proper way.

Oh, okay. Corroborating, I see that NetBeans provides an automatic fix which rewrites the getCookie() call to getLookup().lookup(SaveCookie.class).

ghost · 2018-01-11T20:17:38Z

refactoring.api/src/org/netbeans/modules/refactoring/api/RefactoringSession.java

        @Override
        public void stop(ProgressEvent event) {
-            // do not rely on plugins;
+            /* do not rely on plugins; */


Why the change to a multiline comment?

If using the toggle comment feature with a hotkey, it will use single line comments on each line of the whole selected block. So, the thought was, that a multi-line comment would better reflect the true nature of a comment, and not just some blending out.

Line comment was specifically invented for realy short notes and it's faster to type than /* - */ pair. I don't see a point to change it for one-liners. Maybe for long multilines.
It would be more useful to extend the toggle comment feature to:

make a block comment (maybe a different hotkey or an additional modifier)

turn block of line comments into block comment or vice versa

Whether to use multilines or block comments depend on personal or group development style - it's not a defect to fix.

A different hotkey is not a good idea for me. I have reconfigured some actions to use the F1 to F12 keys. And since that is just not enough, I have started to use the Ctrl key in combination with some characters. Just so, that I can easily reach them both, with my left hand only, and without broken fingers. At this side of the keyboard there aren't much unused keys left for reconfiguration. And it would feel unnatural to use two different keystrokes for similar actions. The /**/ commentation mode is already used for documentation. Why not keep it like that, and use // mostly for partial code changes? Since "do not rely on plugins;" can not be evaluated by the compiler, I would recomment using /**/ commentation mode.

ghost · 2018-01-11T20:23:09Z

refactoring.api/src/org/netbeans/modules/refactoring/api/RefactoringSession.java

-                    if (wrapper != null) {
-                        unsetWrappers(commit, wrapper);
+                    if (undoableWrapper != null) {
+                        undoableWrapper.setActive(false, null);


Should if (undoableWrapper != null) { undoableWrapper.setActive(false, null); } be within a finally block?

It's ok as it is.

ghost · 2018-01-11T20:23:37Z

refactoring.api/src/org/netbeans/modules/refactoring/api/RefactoringSession.java

-            wrapper.close();
-            for (Transaction commit : SPIAccessor.DEFAULT.getCommits(bag)) {
-                SPIAccessor.DEFAULT.sum(commit);
+                undoableWrapper.setActive(false, null);


Similar to before, should undoableWrapper.setActive(false, null); be within a finally block?

It was not before. Probably it will be of use, if you explain it to me.

Sorry, I was unclear. By "similar to before", I meant "similar to my previous comment up above".

My apologies. I'm not able to answer that question. I would add things without concrete knowledge about what I'm actually doing.

Which is very true.

ghost · 2018-01-11T20:27:29Z

refactoring.api/src/org/netbeans/modules/refactoring/api/RefactoringSession.java

            }
-
-            while (it.hasPrevious()) {
+            undoableWrapper.close();


Should undoableWrapper.close(); be within a finally block?

Probably. Under the circumstance, that the code from within line 228 to 232 throws an unexpected exception. I'm not sure, if that is even possible. But it is a potential pitfall. I like your approach.

ghost · 2018-01-11T20:28:04Z

refactoring.api/src/org/netbeans/modules/refactoring/api/RefactoringSession.java

                    }
+                });
+                if (undoableWrapper != null) {
+                    undoableWrapper.close();


Should if (undoableWrapper != null) { undoableWrapper.close(); } be within a finally block?

There are several nested try and finally block in the source. I haven't touched the overall structure of it.

ghost · 2018-01-11T20:31:45Z

refactoring.api/src/org/netbeans/modules/refactoring/api/RefactoringSession.java

-        long time = System.currentTimeMillis();
-
+        final long start = System.currentTimeMillis();
        Iterator it = internalList.iterator();


The type of it could be Iterator<RefactoringElementImplementation>.

It's ok as it is.

If this PR is to improve readability, it should use type params when possible to eliminate (unnecessary) typecasts. Please add it.

ghost · 2018-01-11T20:32:41Z

refactoring.api/src/org/netbeans/modules/refactoring/api/RefactoringSession.java

        try {
-            ListIterator it = internalList.listIterator(internalList.size());
-            fireProgressListenerStart(0, internalList.size()+1);
+            ListIterator internalListIterator = internalList.listIterator(internalList.size());


The type of internalListIterator could be ListIterator<RefactoringElementImplementation>. This should allow the type cast a few lines down to be removed.

It shall be done. Thank you!

ghost · 2018-01-11T20:33:25Z

refactoring.api/src/org/netbeans/modules/refactoring/api/RefactoringSession.java

-                if (element.isEnabled() && !((element.getStatus() == RefactoringElement.GUARDED) || (element.getStatus() == RefactoringElement.READ_ONLY))) {
-                    element.undoChange();
-                }
+                RefactoringElementImplementation element = (RefactoringElementImplementation) internalListIterator.previous();


The type cast to RefactoringElementImplementation could be removed after parameterizing internalListIterator.

Well spotted. Not bad!

ghost · 2018-01-11T20:34:48Z

refactoring.api/src/org/netbeans/modules/refactoring/api/RefactoringSession.java

        public Iterator<RefactoringElement> iterator() {
            return new Iterator() {
-                //private final Iterator<RefactoringElementImplementation> inner = internalList.iterator();
+


Maybe delete the trailing spaces as well?

It's a new anonymous inner class implementation, right? The members can be separated by a newline from the enclosing curly braces. But they don't have to?

I'm fine with the blank lines. It just looked like you were deleting end-of-line whitespace, so the 16 trailing spaces on the new line 350 could be deleted as well.

vieiro · 2018-01-11T22:02:27Z

refactoring.api/src/org/netbeans/modules/refactoring/api/RefactoringSession.java

+        boolean enabled = element.isEnabled();
+        boolean guarded = element.getStatus() == RefactoringElement.GUARDED;
+        boolean readOnly = element.getStatus() == RefactoringElement.READ_ONLY;
+        if (enabled && !(guarded || readOnly)) {


Hi Carsten, this computes "enabled", "guarded" and "readOnly" even when "element.isEnabled()" is false. The original expression
element.isEnabled() && !((element.getStatus() == RefactoringElement.GUARDED) || (element.getStatus() == RefactoringElement.READ_ONLY))
does not, though.

Right, vieiro. I wanted to improve readability, and it resulted in a performance decrease. Haven't even noticed. My bad. Performance is also affected by the added private function call. Before the change, it was done with a redundant inlining of code. Which isn't to bad, but it is not easy to understand.

you can count on private methods calls being inlined by optimizing VM, this is not counted as perf decrease.

You can also can

if (!element.isEnabled()) { return; } // .... if (readOnly || guarded) { return; }

or

if (!enabled || guarded || readOnly) { return; }

Which saves you complex conditions and nested blocks.

vieiro · 2018-01-11T23:18:30Z

Hi @cp7781. Thanks for the PR. Are these changes related to an issue? if so then would you please add an issue to JIRA?
Thanks,
Antonio

ghost · 2018-01-12T01:29:25Z

Thank you for speaking for me, Antonio Vieiro.

svatoun

Overall, I would rather avoid "beautification only" PRs like this. If one examines history later (when analyzing a bug), such "whitespace only" changes add processing time to verify that no semantic change was introduced.

There are only a few changes which in fact improve readability (simple loops onto forEach etc, curly braces in ifs).

svatoun · 2018-01-12T08:46:13Z

refactoring.api/src/org/netbeans/modules/refactoring/api/RefactoringSession.java

+        boolean enabled = element.isEnabled();
+        boolean guarded = element.getStatus() == RefactoringElement.GUARDED;
+        boolean readOnly = element.getStatus() == RefactoringElement.READ_ONLY;
+        if (enabled && !(guarded || readOnly)) {


you can count on private methods calls being inlined by optimizing VM, this is not counted as perf decrease.

You can also can

if (!element.isEnabled()) { return; } // .... if (readOnly || guarded) { return; }

or

if (!enabled || guarded || readOnly) { return; }

Which saves you complex conditions and nested blocks.

svatoun · 2018-01-12T08:51:57Z

refactoring.api/src/org/netbeans/modules/refactoring/api/RefactoringSession.java

+                UndoableWrapper undoableWrapper = MimeLookup.getLookup("").lookup(UndoableWrapper.class);
+                commits.forEach(commit -> {
+                    if (undoableWrapper != null) {
+                        undoableWrapper.setActive(true, this);


Is there a reason to inline setWrappers ? Wouldn't it be better the other way around, to move null check into (un)setWrappers ?

svatoun · 2018-01-12T08:56:48Z

refactoring.api/src/org/netbeans/modules/refactoring/api/RefactoringSession.java

-        long time = System.currentTimeMillis();
-
+        final long start = System.currentTimeMillis();
        Iterator it = internalList.iterator();


If this PR is to improve readability, it should use type params when possible to eliminate (unnecessary) typecasts. Please add it.

svatoun · 2018-01-12T09:01:02Z

refactoring.api/src/org/netbeans/modules/refactoring/api/RefactoringSession.java

-                    fileChange.performChange();
-                }
-            }
+            SPIAccessor.DEFAULT.getFileChanges(bag).stream().filter(fileChange -> fileChange.isEnabled()).forEachOrdered(fileChange -> fileChange.performChange());


Well, I sort of doubt that this stream syntax improves readability over (more verbose) for or forEach. But please make a breakline between the individual processing steps, like this:

SPIAccessor.DEFAULT.getFileChanges(bag).stream(). filter(fileChange -> fileChange.isEnabled()). forEachOrdered(fileChange -> fileChange.performChange());

Which can be usually scanned faster. Debugging of streamized code is a real pain, BTW.

You are right. Would prefer, if the dot is placed at the new line, in front of the method call.

Right. I think that placing the dot first and indenting twice is more standard:

SPIAccessor.DEFAULT.getFileChanges(bag).stream() .filter(fileChange -> fileChange.isEnabled()) .forEachOrdered(fileChange -> fileChange.performChange());

Yes, thanks. I have bad old habits

svatoun · 2018-01-12T09:06:25Z

refactoring.api/src/org/netbeans/modules/refactoring/api/RefactoringSession.java

-            time = System.currentTimeMillis() - time;
-            timer.log(Level.FINE, "refactoringSession.doRefactoring", new Object[] { description, RefactoringSession.this, time } );
+            final long timeTaken = System.currentTimeMillis() - start;
+            timer.log(Level.FINE, "refactoringSession.doRefactoring", new Object[]{description, RefactoringSession.this, timeTaken});


Considering most of the PR fixes whitespaces, there should be whitespaces before/after { to separate from type and array content, and before }.

I don't know, what a PR fix is. But to separate things, to get a better overview, seems to be a good idea.

svatoun · 2018-01-12T09:06:53Z

refactoring.api/src/org/netbeans/modules/refactoring/api/RefactoringSession.java

-            }
-            UndoableWrapper wrapper = MimeLookup.getLookup("").lookup(UndoableWrapper.class);
+            SPIAccessor.DEFAULT.getCommits(bag).forEach(commit -> SPIAccessor.DEFAULT.check(commit, true));
+            UndoableWrapper undoableWrapper = MimeLookup.getLookup("").lookup(UndoableWrapper.class);


// NOI18N marker missing.

There is no string, which needs to be translated. Why adding a comment?

The // NOI18N indicates that the string does not need to be translated.

I believe that some devs have a plugin which places warnings on all strings that are not marked "NOI18N" as a way of verifying that all strings either do not need translation or are localizable.

svatoun · 2018-01-12T09:14:25Z

refactoring.api/src/org/netbeans/modules/refactoring/api/RefactoringSession.java

                public RefactoringElement next() {
                    if (index < internalList.size()) {
-                        return new RefactoringElement(internalList.get(index++));
+                        RefactoringElementImplementation implementation = internalList.get(index);


Coding style in effect does not prohibit pre/postfix increments.

No. That was not the intention. Prefix or postfix operators make loops far more readable in less abstract languages. Java has inherited, adopted and changed the culture, how humans understand code.

OK, then why this change of one-liner return to 3 lines, when the original code did not violate coding conventions ?

I may have seen white rabbits. Changed it back, and no complaints at all. Sometimes, mysterious things happen, and I just try to get used to it, alright? You are right. Thank you for the support.

geertjanw · 2018-01-12T09:22:37Z

A lot of people are spending a lot of time reviewing this pull request. It is unclear why this pull request exists. It seems this is a cosmetic refactoring of a random file in the Apache NetBeans Git repo. We do need to set up guidelines for pull requests and surely one of those will be that we need to have an issue number related to a pull request, i.e., an actual problem that we'd like to have fixed, before fixing it.

ghost · 2018-01-12T16:55:03Z

The project needs far more developers refactoring their work constantly. Especially at the core product. If the overall structure crumbles and falls, it will be just another waste of time in the history of mankind.

ghost · 2018-01-12T16:58:29Z

I think that fixing all of the warnings actually helps. When bug fixing, it is distracting to see all of the yellow warning indicator lines in the sidebar. It makes it more difficult to see what you've changed, as the light green, light blue, and pink added/changed/deleted indicator lines are hidden by the yellow lines. It also makes it more difficult to see the locations of breakpoints that you have set. For example, here is the bar for a file in which I have set two breakpoints and made changes to two parts of the file:

One other benefit to general cleanup (in particular, supplying type information on generics) is that it cuts down on the voluminous output of build warnings.

I have experimented with submitting the "cleanups" in the same patch as a bug fix (e.g. PR #287), but I don't like this approach. One must be in the mood for examining a lot of small changes that result from cleanup. This work is distracting from reviewing the meat of the patch.

ghost · 2018-01-12T17:31:31Z

Sure, cleanup also opens the possibility to oversimplify important parts. Chances are pretty good, that art and skilled craftsmanship end in the trash bin. Lost forever, because the translation of the translation, done by novices like me, without real responsibility, becomes to complicated. That is, what will happen. Either take it for granted, or keep your eyes open the whole night.

the refactoring methods work on three different class member collections, which all contain some sort of RefactoringElement; it is not completely clear, why they are there, and what they are doing; looks like file changes, commits, and a mysterious inner list; one of the refactoring method is full of try and finally blocks, but they don't catch exceptions by itself;

vieiro · 2018-01-13T07:50:53Z

Hi,
Thanks for this PR. These changes seem to improve readability and maintainability, and not a real user problem, so I'm adding the "Cosmetics" label.
I'm also adding the "work in progress" label. Please request a review whenever you're done.
Thanks,
Antonio

ghost · 2018-01-13T09:52:51Z

Cosmetics sound pretty good. Don't they? It's not just fonts and stuff. Good morning, spain.

ghost · 2018-01-13T15:53:40Z

Can anyone tell me, were to get rid of that unit tests? I mean, now it looks like there is a reference somewhere, which needs to be fixed. And actually, I don't want to write unit tests for unit test routines, just because it means business.

ghost · 2018-01-13T16:47:00Z

I think 22e386e needs to be reverted, or please explain why you deleted those unit tests.

Also, I think 1ce564b is starting to go overboard. Some thoughts:

Changing
```
fireProgressListenerStart(0, COMMIT_STEPS + commits.size() * COMMIT_STEPS + 1);
```
to:
```
{
    int count = COMMIT_STEPS + commits.size() * COMMIT_STEPS + 1;
    fireProgressListenerStart(0, count);
}
```
and the other similar changes are unnecessary, in my opinion. I like the introduction of a variable for the result of DataObject.getRegistry().getModified() because it was unclear that this has type DataObject[] instead of some type of Iterable; but, other introduced variables seem unnecessary.
I don't think that reallyDoRefactoring() and reallyUndoRefactoring() should be inlined. Don't get me wrong, I appreciate the reason why you did this. However, I think that the original pattern should be kept, as it is a perfectly acceptable pattern to use.
Arrays.asList(...).stream() can be replaced with Stream.of(...).
With the exception of the JavaDoc comment on performChange(), I think that the other new comments are unnecessary and should be removed.

One final thought: I think that commit messages should not be cutesy or contain jokes (e.g. "against the order of maw") because the developers working on this project come from different backgrounds and may not get all of the references. I had to search "order of maw" to figure out that it is a Warcraft reference (I think? still not sure...).

This reverts commit 22e386e.

ghost · 2018-01-13T18:40:38Z

For now, I wouldn't mind those tests, Daniel. There is some package missing, or something. And it turns red, if the plugin is opened. Just want to fix that.

This computation of the count variable indeed looks really awkward. What exactly is getting computed?
```
{
    int count = COMMIT_STEPS + commits.size() * COMMIT_STEPS + 1;
    fireProgressListenerStart(0, count);
}
```
The fireProgressListenerStep() method is getting called at line 117 and 163. Once in a loop over items in the internalList member variable, and once at the end of a try block. There seems to be no connection to the call of commits.size() in the code listing above. So, this part might be corrupted?
Using lambda expressions is an elegant way to streamline code. Before change, there were two private functions in place, which were both called just once. It not only clutters the navigator window, it also distracts attention away from the requirements.
Thank you for mentioning Stream.of(...).
Doctor, can you please help me? The people say, I'm an idiot, and they are constantly laughing at me.

Order of maw means breakfast. I had toast, honey and milk. And a nice girl was talking to me meanwhile. Life is just to good for waking up early.

sdedic · 2018-01-13T19:07:14Z

Keep in mind that usage of Lambda expression also totally breaks fix-and-continue during debugging. It's a nice syntax suggar but until Hotspot folks fix the VM, class with lambdas does not allow to incrementally fix the code.

As for unit tests: they run on the master. So the only sane approach is to fix the code so that tests run again. But this PR is WIP, so OK. But before the PR is reviewed, I recommend to discard that commit from history.

The PR starts being hard to read, mixing (intentionally just) formatting changes with refactorings which actually have potential to break something. I recommend to separate those two actions into either separate commits, or separate PRs - it's easier to verify that refactoring did not change semantic if 90% of file is not whitespace-changed.

As for commit messages (personal opinion): I will not approve PR with messages like "for the queen", "fixing errors" (which actually deletes test code), or "order of maw". I believe that added confusion which outweights the fun of reading them.

ghost · 2018-01-13T21:59:00Z

Debugging, like making use of assert statements, is a relic of the past. Modern software in general has become far way to complex for being used with such features. But take it as my humble opinion. I've almost no idea, how a real project would look like, and what possibly can be done, if used cleverly by a true master.

ghost · 2018-01-14T14:26:51Z

So, just to make sure. The tags code-cleanup and work-in-progress seem to mark somewhat suspicious pull requests. You likely want to concentrate on other things; probably more useful stuff, than review and education. I will close this. Good luck!

cp7781 added 2 commits January 10, 2018 21:33

my first refactoring session

e397559

tried to attend to the highlighted items, guided by the IDE

code cleanup

4ca7524

ghost reviewed Jan 11, 2018

View reviewed changes

vieiro reviewed Jan 11, 2018

View reviewed changes

type safety by generics

862ad2d

svatoun suggested changes Jan 12, 2018

View reviewed changes

cp7781 added 2 commits January 12, 2018 18:55

stick to the natural order of things

d195fc6

vieiro added Code cleanup work-in-progress labels Jan 13, 2018

cp7781 added 2 commits January 13, 2018 11:24

against the order of maw

e5f243d

fixing error messages

22e386e

Revert "fixing error messages"

24d766e

This reverts commit 22e386e.

for the queen

bc454b4

ghost closed this Jan 14, 2018

This pull request was closed.

my first refactoring session #361

my first refactoring session #361

Uh oh!

Conversation

ghost commented Jan 10, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost Jan 11, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Jan 12, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Jan 11, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Jan 12, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vieiro commented Jan 11, 2018

Uh oh!

ghost commented Jan 12, 2018

ghost commented Jan 10, 2018 •

edited by ghost

Loading

ghost Jan 11, 2018 •

edited by ghost

Loading

ghost Jan 12, 2018 •

edited by ghost

Loading

ghost Jan 11, 2018 •

edited by ghost

Loading

ghost Jan 12, 2018 •

edited by ghost

Loading