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

Perhaps {0} could be replaced by a local variable. #420

Closed
carlwilson opened this issue Mar 26, 2019 · 4 comments
Closed

Perhaps {0} could be replaced by a local variable. #420

carlwilson opened this issue Mar 26, 2019 · 4 comments
Assignees
Labels
bug A product defect that needs fixing P3 Low priority bugs

Comments

@carlwilson
Copy link
Member

There are 31 occurrences of this issue.
Why is this an issue?
Since: PMD 3.1

Fields whose scopes are limited to just single methods do not rely on the containing object to provide them to other methods. They may be better implemented as local variables within those methods.

Example(s):

public class Foo {
    private int x;  // no reason to exist at the Foo instance level
    public void foo(int y) {
     x = y + 5;
     return x;
    }
}

A full list of occurrences can be found on Codacy here

@carlwilson carlwilson added bug A product defect that needs fixing P3 Low priority bugs labels Mar 26, 2019
@carlwilson
Copy link
Member Author

Hi @rgfeldman I thought I'd start you on a gentle(ish) one but it does require a little judgement. The issue concerns class member variables that are only used by a single method. These might be better implemented as local variables in the individual methods. This isn't a beginners issue as it requires a little judgment but it's still a nice starter. There's not too many occurrences but I'd still suggest breaking the set up into multiple pull requests, one for each JHOVE module would seem sensible.

Thanks for helping and good luck.

@rgfeldman
Copy link

jhove-apps/src/main/java/edu/harvard//hul/ois/jhove/viewer/InfoWindow.java
Perhaps '_closeItem' could be replaced by a local variable.

There can be argument for keeping the definitions of the two JMenu class variables together (With _saveItem) in the class member variable. To Make _save Item local as well may be possible, but would be additional work. Not changing this for now
xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
jhove-apps/src/main/java/edu/harvard/hul/ois/jhove/viewer/RepTreeRoot.java
Perhaps '_base' could be replaced by a local variable.

The _base variable was not needed at all, it was sent in as a parameter to a member function
and then assigned and used right away (_base = base). See no need for this variable, removing _base class member from code, but kept base member variable.
xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
...more comments to come....

@carlwilson
Copy link
Member Author

Hi @rgfeldman feel free to use your skill and judgement, agree that not all will be best as local vars.

rgfeldman pushed a commit to rgfeldman/jhove that referenced this issue Apr 10, 2019
rgfeldman added a commit to rgfeldman/jhove that referenced this issue Apr 12, 2019
rgfeldman added a commit to rgfeldman/jhove that referenced this issue Apr 12, 2019
rgfeldman added a commit to rgfeldman/jhove that referenced this issue Apr 12, 2019
rgfeldman added a commit to rgfeldman/jhove that referenced this issue Apr 12, 2019
rgfeldman added a commit to rgfeldman/jhove that referenced this issue Apr 12, 2019
rgfeldman added a commit to rgfeldman/jhove that referenced this issue Apr 12, 2019
rgfeldman added a commit to rgfeldman/jhove that referenced this issue Apr 12, 2019
rgfeldman added a commit to rgfeldman/jhove that referenced this issue Apr 12, 2019
rgfeldman added a commit to rgfeldman/jhove that referenced this issue Apr 12, 2019
@rgfeldman
Copy link

few more comments about the analysis and work done:

##########################################################
jhove-ext-modules/src/main/java/com/mcgath/jhove/module/png/ChrmChunk.java
moved variables indicated (applicable declaration block) to local scope
##############################################################################
jhove-ext-modules/src/main/java/com/mcgath/jhove/module/png/IhdrChunk.java
moved variables indicated (applicable declaration block) to local scope
##############################################################################
jhove-modules/jpeg-hul/src/main/java/edu/harvard/hul/ois/jhove/module/jpeg/JpegExif.java
removal of un-used class variable 'JpegModule module'.
Note: this class is instantiated by another class, and Also had to remove the parameter there.
Looked safe to me, as the variable appeared to not be used, but hope this does not break anything.
##############################################################################
jhove-modules/jpeg2000-hul/src/main/java/edu/harvard/hul/ois/jhove/module/jpeg2000/ContCodestream.java
removal of unneccessary class variable Codestream _codestream.
It was a privately scoped variableit was sent in as a parameter to a member function
and then assigned and used right away. Used only in one member function, we can use the parameter variable
locally rather than using a class variable.
############################################################################
jhove-modules/jpeg2000-hul/src/main/java/edu/harvard/hul/ois/jhove/module/jpeg2000/FragmentInputStream.java
removed bufsize, does not appear to be used. There are already locally scopped bufsize variables,
that seems all that is needed here.
#############################################################################
jhove-modules/pdf-hul/src/main/java/edu/harvard/hul/ois/jhove/module/pdf/Literal.java
_parenLevel moved to variable with local scope and renamed without leading underscore,
see no reason for scope in whole class, especially since it is a value incremented in a loop.
##############################################################################
jhove-modules/pdf-hul/src/main/java/edu/harvard/hul/ois/jhove/module/pdf/PdfFlateInputStream.java
moved both vars here (int bpc int columns)
and to be local in scope to member function where it is used
#############################################################################
jhove-modules/pdf-hul/src/main/java/edu/harvard/hul/ois/jhove/module/pdf/StructureElement.java
keeping this as is, something looks fishy about this design and treatment of this variable.
Existing code may be fine, but may require additional analysis before changing the scope of this
variable (dont want to break anything here at the moment). No changes made.
##############################################################################
jhove-core/src/main/java/edu/harvard/hul/ois/jhove/AESAudioMetadata.java
Removed variable _primaryIdentifierOtherType
that apparently is assigned but not used anywhere
##############################################################################
jhove-modules/jpeg2000-hul/src/main/java/edu/harvard/hul/ois/jhove/module/jpeg2000/TilePart.java
keeping as is, object that contains this variable is very small, the variable is intrinsic to the
and the variable in question is accessible to public. No changes made.
#############################################################################hove-modules/pdf-hul/src/main/java/edu/harvard/hul/ois/jhove/module/pdf/CrossRefStream.java
Commenting out filters[] declaration (but not deleting the trace of this variable from code),
Flagged variable is currently not being used, and there is some already existing comment in the code
that questions as to why this isnt being used.

_bytesperentry moved to local area in code, where it is used and renamed without leading underscore
#############################################################################
jhove-modules/pdf-hul/src/main/java/edu/harvard/hul/ois/jhove/module/pdf/Literal.java
State _state moved to locally scoped variable, removed the underscore in the filename
#############################################################################
jhove-modules/wave-hul/src/main/java/edu/harvard/hul/ois/jhove/module/wave/ListInfoTextChunk.java
commenting out, does not appear to be used.

@carlwilson carlwilson removed this from the Hack week tasks milestone Apr 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A product defect that needs fixing P3 Low priority bugs
Projects
None yet
Development

No branches or pull requests

2 participants