Some bulk refactorings #282

oreissig · 2016-03-06T15:09:14Z

I went through the codebase of the root project and did the following refactorings:

make private methods static when they are plain functions
add @deprecated when javadoc says the method is @deprecated
remove unused code
avoid synthetic accessors by making private fields/methods package-private
make private fields final where possible
fix some inconsistent whitespaces

None of this should change the behaviour in any way.

oreissig · 2016-03-06T20:19:14Z

Rebased from master due to conflicts.

PascalSchumacher · 2016-03-07T20:19:43Z

Thanks for the pull request. :)

I already cherry-picked a0aed6c which fixes some mistakes.

I support merging the rest of the commits, although I'm not sure how much a0aed6c or a032ed6 add. With 0a78058 there is a small change that somebody calls the methods from dynamic groovy, but of course it is important to remove unused stuff.

I'm thankful for opinions of other commiters on this.

oreissig · 2016-03-07T20:28:18Z

This one particular setSize() method in 0a78058 I actually deleted initially, but added it again when I went through the diffs and noticed the comment above. I decided to make the method @deprecated as a neat trick to indicate in an IDE that this should not be called if written by accident. If you don't like this trick, I'll just leave the method as is.

jwagenleitner · 2016-03-07T22:49:43Z

For commit a032ed6 (avoid synthetic accessors by making private fields/methods package-private), I think removing private from members of private nested classes where the member is accessed by the top-level class would be ok. However, I don't think members of top-level public classes should be changed from private to package-private. If they weren't designed to be used by other classes in the package then they shouldn't be made visible just to avoid a synthetic method. So overall I'd be -1 for this particular commit.

The other commits look good to me.

oreissig · 2016-03-08T07:11:44Z

I also thought about the widening of scope, that may be unintended, but if there's a synthetic accessor method generated, it actually is made available inside the package anyway? Scope-wise there's not much of a difference between a package-private field and a package-private getter, or is there one?

jwagenleitner · 2016-03-08T16:56:42Z

but if there's a synthetic accessor method generated, it actually is made available inside the package anyway?

To my knowledge the synthetic accessors are inserted by the Java compiler so that top-level and inner classes can access private members of each other. From a coding perspective it is not really feasible for another class in the package to use these compiler generated accessors (i.e., they wont show up in IDE code completion). By widening the scope of the members of the enclosing class encapsulation is being broken in order to avoid having a few synthetic accessors; the only benefit might be slight performance increase and that is even very questionable. But many people work on the codebase and encapsulation is more important in my opinion in this context. If someone working in the code see they can access the member they may think it was designed to be accessed by classes in the package when it actually was not.

Scope-wise there's not much of a difference between a package-private field and a package-private getter, or is there one?

No I don't believe so. In my original comment I was referring to synthetic accessors in general and not just those generated for methods. I think again it comes back to encapsulation and breaking it just to avoid the synthetic accessors is not a good thing.

oreissig · 2016-03-08T19:12:41Z

Good point about IDEs respecting the synthetic flag, thanks! So I'll revert all the scoping changes in non-inner classes. For private inner classes the scope should not matter that much, since they as a whole are not accessible to any other classes, therefore I leave my changes there.

…ivate

oreissig · 2016-03-08T21:06:25Z

Rebased from trunk due to conflicts.

The only cases left where I changed from private to package-private is for private inner classes now.

jwagenleitner · 2016-03-09T17:13:33Z

Because many of the commits touch the same files it is difficult to review just the changes for the synthetic accessors/package-private. I think for bulk changes like this, even if minor, separate PR's would be easier to review and manage.

PascalSchumacher · 2016-03-10T18:41:39Z

I agree in the future I would prefer separate PR's.

Can you please squash c7fec36 and fcbe33e? Thanks!

oreissig · 2016-03-10T19:15:10Z

Yes I already realized this was a bad idea. I would've love to just fixup the initial scope changing commit, but could not do so without major effort because it conflicts with the other commit adding final here and there, that has been done in the meantime :-/

PascalSchumacher · 2016-03-10T19:39:56Z

Another problem with doing so many changes is that there are a lot of merge conflicts when I try to merge the changes into the 2_4_X branch, but without merging it's highly likely that there will be conflicts when backporting bugfixes. :(

oreissig · 2016-03-10T19:44:14Z

If you want I can go through again and restructure the changes. How do you like it? Individual PRs for each of the commits (except the fixup)? Should those individual PRs only contain one commit each, or do you like it to be split up by some metric?

PascalSchumacher · 2016-03-10T20:02:07Z

No further thinking about it I'm not sure if multiple pull request will really help in this case. I guess the pull request would have massive merge conflicts with each other.

I think 69ade9e it the most valuable commit, because removing code dead code is good for readability and maintainability. I do not know if there is an easy way to untangle it from the rest? c3ad829 is also nice. Of course the other commit also have value.

jwagenleitner · 2016-03-10T20:35:07Z

I think that bulk refactorings like these are best done one at a time (serially) with each as it's own PR with a single commit focused on the one change and agree that the unused code commit would be a good one to start with. For me I'd wait for it to be merged before starting work on the next PR if lots of the same files are to be touched, and work on them in order of most likely to least likely to be merged. I think that would avoid the headaches of dealing with merge conflicts.

PascalSchumacher · 2016-03-12T10:28:27Z

@oreissig I rebased the remove unused code commit. Thanks!

paulk-asert · 2017-03-21T02:43:40Z

src/main/groovy/lang/GroovyCodeSource.java

        this(uri.toURL());
    }

-    public GroovyCodeSource(URL url) throws IOException {


I only just spotted this now. While "unused" in the sense that this method didn't throw this exception anymore (I presume it did in the past), removing the exception breaks contract/source code compatibility. :-( I know it is a result of how checked exceptions are handled on the JVM but we should have not done this in a point release - all good to know in hindsight. Anyway, it isn't clear that reverting after a year is going to help things so I'm thinking of just documenting for now with a Jira issue. Apologies if this has already been raised elsewhere.

d'oh, I totally forgot about that :(

blackdrag · 2017-03-22T21:21:20Z

src/main/groovy/grape/GrapeIvy.groovy

+    final Set<IvyGrabRecord> grabRecordsForCurrDependencies = new LinkedHashSet<IvyGrabRecord>()
    // we keep the settings so that addResolver can add to the resolver chain
-    IvySettings settings
+    final IvySettings settings


not sure those finals are ok, it means there will be no getter for these anymore. So I would like to hear from for example Paul that these are not supposed to have getter

blackdrag · 2017-03-22T21:23:12Z

src/main/groovy/lang/ObjectRange.java

-    private void setSize(int size) {
+    @Deprecated
+    void setSize(int size) {
        throw new UnsupportedOperationException("size must not be changed");


why is it deprecated again?

blackdrag · 2017-03-22T21:23:45Z

src/main/groovy/lang/ProxyMetaClass.java

     * convenience factory method for the most usual case.
     */
-    public static ProxyMetaClass getInstance(Class theClass) throws IntrospectionException {
+    public static ProxyMetaClass getInstance(Class theClass) {


removal of checked exception in public method

blackdrag · 2017-03-22T21:24:12Z

src/main/groovy/lang/ProxyMetaClass.java

    /**
     * @param adaptee the MetaClass to decorate with interceptability
     */
-    public ProxyMetaClass(MetaClassRegistry registry, Class theClass, MetaClass adaptee) throws IntrospectionException {


removal of checked exception in public method

blackdrag · 2017-03-22T21:34:44Z

src/main/org/codehaus/groovy/reflection/GeneratedMetaMethod.java

        }

-        public static List<DgmMethodRecord> loadDgmInfo() throws IOException, ClassNotFoundException {
+        public static List<DgmMethodRecord> loadDgmInfo() throws IOException {


removal of checked exception from public api

blackdrag · 2017-03-22T21:40:19Z

src/main/org/codehaus/groovy/tools/DgmConverter.java

 public class DgmConverter implements Opcodes {

-    public static void main(String[] args) throws IOException, ClassNotFoundException {
+    public static void main(String[] args) throws IOException {


even though this is not likely to cause a problem, still removal of checked exception on public method

blackdrag · 2017-03-22T21:41:23Z

src/main/org/codehaus/groovy/tools/shell/IO.java

     * Flush both output streams.
     */
-    public void flush() throws IOException {
+    public void flush() {


removal of checked exception on public method

blackdrag · 2017-03-22T21:44:54Z

src/main/org/codehaus/groovy/vmplugin/v7/IndyArrayAccess.java

-        return 0;
-    }
-
-    private static int normalizeIndex(Object array, int i) {


normalizeIndex is used via MethodHandle in line 39. you cannot remove these

oreissig added 6 commits March 8, 2016 20:50

make private methods static when they are plain functions

13082d4

remove unused code

69ade9e

avoid synthetic accessors by making private fields/methods package-pr…

c7fec36

…ivate

make private fields final where possible

c3ad829

fix some inconsistent whitespaces

e808801

revert scope widening for non-inner or public inner classes

fcbe33e

asfgit pushed a commit that referenced this pull request Mar 12, 2016

remove unused code (closes #282)

972745b

asfgit closed this in 78b6cc5 Mar 12, 2016

This was referenced Mar 12, 2016

make private methods static when they are plain functions #290

Closed

remove unused or obsolete code #291

Closed

make private fields final where possible #292

Closed

paulk-asert reviewed Mar 21, 2017

View reviewed changes

blackdrag reviewed Mar 22, 2017

View reviewed changes

Some bulk refactorings #282

Some bulk refactorings #282

Uh oh!

Conversation

oreissig commented Mar 6, 2016

Uh oh!

oreissig commented Mar 6, 2016

Uh oh!

PascalSchumacher commented Mar 7, 2016

Uh oh!

oreissig commented Mar 7, 2016

Uh oh!

jwagenleitner commented Mar 7, 2016

Uh oh!

oreissig commented Mar 8, 2016

Uh oh!

jwagenleitner commented Mar 8, 2016

Uh oh!

oreissig commented Mar 8, 2016

Uh oh!

oreissig commented Mar 8, 2016

Uh oh!

jwagenleitner commented Mar 9, 2016

Uh oh!

PascalSchumacher commented Mar 10, 2016

Uh oh!

oreissig commented Mar 10, 2016

Uh oh!

PascalSchumacher commented Mar 10, 2016

Uh oh!

oreissig commented Mar 10, 2016

Uh oh!

PascalSchumacher commented Mar 10, 2016

Uh oh!

jwagenleitner commented Mar 10, 2016

Uh oh!

PascalSchumacher commented Mar 12, 2016

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants