PMD exclude cleanup #2777

TWiStErRob · 2018-01-01T04:28:50Z

I went through a few of the suppressed/excluded PMD warnings.

Generic approach: After fixing the fixable... suppress only the most specific things, so future modifications cannot lurk in regressions to the same problem. (i.e. move generic <exclude> to @SuppressWarnings; and move @SW from class to method, or from method to local variable/statement).

Example where this really shows: @SuppressWarnings("unused") means that the method is probably not tested.

AvoidCatchingThrowable/AvoidCatchingGenericException: It's clear that this is bad, but we have a number of cases where it makes sense and a blanket ban is irritating.

There were actually a few instances where a blanket Exception was used, but there were no checked exceptions thrown. Invoke generic approach.

AvoidCatchingGenericException: This is redundant, also caught with AvoidCatchingNPEs.

Agree, suppressed NPEs, and enabled the rule. It is weird that Throwable is not a generic "exception", but NPEs are.
I guess these two rules may need to be merged together in PMD and made properly parameterizable.

AvoidUsingVolatile: Used frequently in the singleton pattern.

Only half the usages are Singleton-ish. Those could be filtered with .[@Private='true' and @Static='true'].

ShortClassName: This is not always true.
ShortMethodName/ShortVariable: A good idea but we have tons of violations.

Suppressed common usages in config, some special ones in code, and fixed the rest so we can enable these rules. Some "global suppressions" could be resolved with a simple search and replace (all files, "match case", "whole words"); similar to cb → callback.

ExcessiveMethodLength, ExcessivePublicCount, ExcessiveClassLength: TODO: explore these further

Downsampler: extracted some methods to shorten the methods and restrict scope of some calculations. Some of these methods are static so they can be moved out and tested individually.
See TODO this comment is out of place, probably belongs to one of getImageSpecificSize branches, I couldn't figure that out.
RequestBuilder: had a very long method, buildThumbnailRequestRecursive which I split into 3 and documented what's going on (also reordered the private method parameters to be consistent within class). Due to this the class length increased a bit and now it's a too long class. However I think it's possible to extract half of the methods out to a separate class as RequestBuilder is doing two things: gathering the data from the user of Glide AND calculating how to deal with that data.
RequestOptions is a really long class, not much we can do about it probably because of the myriad of options.

These refactors look ugly, but they're all "extract method"s really. Use IDEA's diff tool to see.

CouplingBetweenObjects

Glide: split out a factory that creates the Registry, this splits the dependencies of Glide into half and allows for "better" code to create the registry. I split the register/append calls up along the /**/ comments in the method. Strangely ExcessiveMethodLength appeared as I extracted the registry related stuff to a separate class: pmd/pmd#825; this is why I "needed" to split, not sure if it's better I would probably be ok with leaving it a single method with locals (passing in the shared objects looked strange). Curious what you think about this split.

TooManyStaticImports: Theoretically this might be reasonable but the number of imports probably varies from class to class and this doesn't seem worth the overhead to maintain

This particular violation is actually harder to read with static imports, hard to know if one is enum or int constant. Also there was mixed usage of static and non-static access to ImageType.

TWiStErRob · 2018-01-01T04:32:54Z

@sjudd I just saw you have some potentially related commits, I'll rebase tomorrow. Until then, let me know what you think, I suggest you look at commits individually for review. This is WIP, meaning there are more <excludes I want to check.

…ules Existing reasonable catches are suppressed, and new ones are prevented: One has to think twice i.e. suppress the PMD warning, if they want to catch-all.

fix in GeneratedAppGlideModule, APT always implements this

…rules

…urrences)

… no new instances can be introduced

sjudd

Thanks for doing this!

sjudd · 2018-01-01T07:03:28Z

library/pmd-ruleset.xml

+        ]" />
+      </properties>
+    </rule>
+    <rule ref="category/java/codestyle.xml/ShortClassName">


I'd be inclined to leave this off entirely, I haven't seen an example where this has caught a class that should actually be renamed.

sjudd · 2018-01-01T07:05:18Z

library/pmd-ruleset.xml

@@ -134,11 +170,12 @@
        <!-- TODO: explore these further. -->
        <exclude name="CyclomaticComplexity" />
        <exclude name="NPathComplexity" />
-        <exclude name="ExcessiveMethodLength" />


I'd rather leave these off for now. If we want to refactor, we're better off doing so in areas that we think are important, not just because we've hit an arbitrary limit.

I think the complexity checks are somewhat more interesting.

sjudd · 2018-01-01T07:06:51Z

library/src/main/java/com/bumptech/glide/RequestBuilder.java

@@ -64,7 +67,7 @@
  private TransitionOptions<?, ? super TranscodeType> transitionOptions;

  @Nullable private Object model;
-  // model may occasionally be null, so to enforce that load() was called, put a boolean rather
+  // TODO model may occasionally be null, so to enforce that load() was called, put a boolean rather


This is actually done, but isModelSet is defined below. Probably just move the comment.

sjudd · 2018-01-01T07:07:13Z

library/src/main/java/com/bumptech/glide/RequestBuilder.java

-@SuppressWarnings({"unused", "WeakerAccess"})
+@SuppressWarnings({
+    // Glide has a lot of ways to load models, and can build recursive thumbnail requests.
+    // TODO split these two responsibilities into classes (entry point for latter: `buildRequest`)


I'd remove this, we don't want to allocate multiple classes per request.

sjudd · 2018-01-01T07:08:14Z

library/src/main/java/com/bumptech/glide/load/engine/Engine.java

@@ -129,8 +129,8 @@ public Engine(
   *   <li>Check the current set of actively used resources, return the active resource if
   *   present, and move any newly inactive resources into the memory cache.</li>
   *   <li>Check the memory cache and provide the cached resource if present.</li>
-   *   <li>Check the current set of in progress loads and add the cb to the in progress load if
-   *   one is present.</li>
+   *   <li>Check the current set of in progress loads


Was this intentional?

sjudd · 2018-01-01T07:10:09Z

library/src/main/java/com/bumptech/glide/load/resource/bitmap/DownsampleStrategy.java

@@ -183,10 +183,10 @@ public SampleSizeRounding getSampleSizeRounding(int sourceWidth, int sourceHeigh
    }
  }

-  private static class None extends DownsampleStrategy {


I'd prefer None (see my comment about short class names).

sjudd · 2018-01-01T07:12:36Z

library/src/main/java/com/bumptech/glide/load/resource/bitmap/TransformationUtils.java

@@ -254,6 +254,7 @@ public static void setAlpha(Bitmap inBitmap, Bitmap outBitmap) {
   *                        returned unmodified.
   * @return The oriented bitmap. May be the imageToOrient without modification, or a new Bitmap.
   */
+  // TODO why do we have this method, it's not used? @Deprecate and remove?


sjudd · 2018-01-01T07:14:29Z

library/src/main/java/com/bumptech/glide/request/RequestOptions.java

@@ -40,7 +40,14 @@
 /**
 * Provides type independent options to customize loads with Glide.
 */
-@SuppressWarnings({"PMD.UseUtilityClass", "unused"})
+@SuppressWarnings({
+    // Glide has a lot of options that can customize the load, it's not feasible to split this.


Another reason to leave most of these off. I think it's relatively apparent where we can and can't do better, though I guess this serves as some degree of documentation?

Again I'm more interested in hearing what people think about classes and how they can be refactored than being warned when I've hit an arbitrary limit of things. Unlike a lot of the other PMD checks, it's pretty apparent when a class is large or has lots of methods/fields/imports etc.

sjudd · 2018-01-07T18:52:22Z

library/src/main/java/com/bumptech/glide/RequestBuilder.java

-          priority,
-          overrideWidth,
-          overrideHeight);
+          target, targetListener, parentCoordinator, requestOptions,


Style nit: leave the parameters on individual lines.

We use:
method(arg1, arg2, arg3); if the method fits on one line.
method(
arg1, arg2, arg3); if the method doesn't fit on one line, but the arguments do
method(
arg1,
arg2,
arg3); if the arguments don't fit on one line.

It's fine to leave places where we don't follow this alone, but we shouldn't revert places where we've already switched to doing this.

sjudd · 2018-01-07T18:55:18Z

library/src/main/java/com/bumptech/glide/RequestBuilder.java

+            overrideWidth,
+            overrideHeight
+        );
+    this.isThumbnailBuilt = true;


It's easier to understand I think if this parameter is checked in the same method where it's used? Can you move the check in https://github.com/bumptech/glide/pull/2777/files#diff-a38a476f7aa7c143d4cde2716bb2b59dR955 to this method?

sjudd · 2018-01-09T05:32:25Z

I tried rebasing this but I'm unable to push to your branch. Fortunately the only conflict is simple to resolve. BitmapEncoder in Glide got an ArrayPool constructor, so that just has to change in the new DefaultRegistryFactory class.

TWiStErRob · 2018-01-09T09:38:58Z

Huh, interesting, I'll clean and respond if I find some time this week/end.

Kongreen · 2020-02-24T17:31:04Z

library/src/main/java/com/bumptech/glide/RequestBuilder.java

-          priority,
-          overrideWidth,
-          overrideHeight);
+          target, targetListener, parentCoordinator, requestOptions,


Kongreen · 2020-02-24T17:32:29Z

library/src/main/java/com/bumptech/glide/RequestBuilder.java

-              overrideWidth,
-              overrideHeight);
-      isThumbnailBuilt = true;
-      // Recursively generate thumbnail requests.


TWiStErRob · 2020-02-24T17:57:57Z

Huh, interesting, I'll clean and respond if I find some time this week/end.

Ooops, clearly forgot about this.

Kongreen · 2020-02-24T18:03:45Z

ок -- Отправлено из Mail.ru для Android понедельник, 24 февраля 2020г., 21:58 +04:00 от Róbert Papp [email protected] :

…

>Huh, interesting, I'll clean and respond if I find some time this week/end. Ooops, clearly forgot about this. — You are receiving this because you commented. Reply to this email directly, view it on GitHub , or unsubscribe .

TWiStErRob added build stability enhancement v4 labels Jan 1, 2018

TWiStErRob self-assigned this Jan 1, 2018

TWiStErRob requested a review from sjudd January 1, 2018 04:28

googlebot added the cla: yes label Jan 1, 2018

TWiStErRob mentioned this pull request Jan 1, 2018

./gradlew clean build --profile produced spurious PMD errors #2776

Closed

TWiStErRob added 2 commits January 1, 2018 15:37

Chrome Developer Tools console snippet for PMD violations

46def23

PMD.AvoidCatchingThrowable/AvoidCatchingGenericException: configure r…

7174c4c

…ules Existing reasonable catches are suppressed, and new ones are prevented: One has to think twice i.e. suppress the PMD warning, if they want to catch-all.

TWiStErRob force-pushed the pmd_cleanup branch 2 times, most recently from c372127 to 0d90947 Compare January 1, 2018 16:54

TWiStErRob added 12 commits January 2, 2018 01:18

PMD.EmptyMethodInAbstractClassShouldBeAbstract: allow if commented

c31a39b

fix in GeneratedAppGlideModule, APT always implements this

PMD.ShortClassName/ShortMethodName: fix violations, and re-configure …

1f53eed

…rules

PMD.ShortVariable: quick win towards enabling it (cb -> callback)

c373720

PMD.ShortVariable: quick win towards enabling it (suppress common occ…

9f02496

…urrences)

PMD.ShortVariable: fix the rest of them

f571861

PMD.ExcessiveMethodLength: Downsampler.calculateScaling

30b2fb0

PMD.ExcessiveMethodLength: Downsampler.decodeFromWrappedStreams

2f51b52

PMD.ExcessiveMethodLength: RequestBuilder.buildThumbnailRequestRecursive

971bf29

PMD.Excessive* is now enabled

72a48f5

PMD.CouplingBetweenObjects: cut Glide class in half

a6292ac

PMD.TooManyStaticImports: qualify ImageType

e767fe7

PMD.AvoidBranchingStatementAsLastInLoop: move suppression to local so…

520deda

… no new instances can be introduced

TWiStErRob force-pushed the pmd_cleanup branch from 4809049 to 41a810d Compare January 2, 2018 01:42

PMD.CallSuperInConstructor: fix violations, ignore extends Object

a871189

TWiStErRob force-pushed the pmd_cleanup branch from 41a810d to a871189 Compare January 2, 2018 01:43

sjudd requested changes Jan 7, 2018

View reviewed changes

Kongreen reviewed Feb 24, 2020

View reviewed changes

sjudd closed this Jun 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PMD exclude cleanup #2777

PMD exclude cleanup #2777

TWiStErRob commented Jan 1, 2018 •

edited

Loading

TWiStErRob commented Jan 1, 2018 •

edited

Loading

sjudd left a comment

sjudd Jan 1, 2018

Kongreen Feb 24, 2020

sjudd Jan 1, 2018

Kongreen Feb 24, 2020

sjudd Jan 1, 2018

Kongreen Feb 24, 2020

sjudd Jan 1, 2018

Kongreen Feb 24, 2020

sjudd Jan 1, 2018

sjudd Jan 1, 2018

sjudd Jan 1, 2018

sjudd Jan 1, 2018

sjudd Jan 7, 2018

sjudd Jan 7, 2018

sjudd commented Jan 9, 2018

TWiStErRob commented Jan 9, 2018 •

edited

Loading

Kongreen Feb 24, 2020

Kongreen Feb 24, 2020

TWiStErRob commented Feb 24, 2020

Kongreen commented Feb 24, 2020 via email

PMD exclude cleanup #2777

PMD exclude cleanup #2777

Conversation

TWiStErRob commented Jan 1, 2018 • edited Loading

TWiStErRob commented Jan 1, 2018 • edited Loading

sjudd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjudd commented Jan 9, 2018

TWiStErRob commented Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TWiStErRob commented Feb 24, 2020

Kongreen commented Feb 24, 2020 via email

TWiStErRob commented Jan 1, 2018 •

edited

Loading

TWiStErRob commented Jan 1, 2018 •

edited

Loading

TWiStErRob commented Jan 9, 2018 •

edited

Loading