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.

Sign up for GitHub

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

Jump to bottom

Update ClassParser.java #406

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update ClassParser.java #406

wants to merge 1 commit into from

Conversation

jjestrada2
Copy link

Refactored ClassParser to Use instanceof for ZIP/JAR Detection

Problem:

The previous implementation used getClass().getName() to detect if an InputStream was from a ZIP/JAR file. This approach relied on string matching, which was inefficient and prone to break if class names changed.

Solution:

  • Replaced reflection-based detection with instanceof checks for ZipInputStream and JarInputStream.
  • Improved maintainability by eliminating unnecessary string operations.
  • Added missing imports for ZipInputStream and JarInputStream.

Testing:

  • Ran mvn clean install successfully.
  • Verified existing tests pass.
  • No breaking changes observed.

@jjestrada2
Update ClassParser.java
579c57c 
Refactor ClassParser to use instanceof for ZIP/JAR detection

Removed reflection-based input stream detection (getClass().getName()).
Replaced with instanceof checks for ZipInputStream and JarInputStream.
Added missing imports for ZipInputStream and JarInputStream.
Improved maintainability by eliminating unnecessary string operations.
Enhanced performance by using a direct type check instead of string comparisons.
@garydgregory
Copy link
Member

The breaking change is what would happen if a class happens to be using that package but is not one of the two specific classes the PR now requires.

The behavior is different, from package testing to 2 specific classes.

I think we can leave this alone for now. Any talk of performance is inappropriate without numbers.

@garydgregory
Copy link
Member

If you're looking for something to do here, you could see why ConstantPoolModuleAccessTestCase fails on Java 23.

@jjestrada2
Copy link
Author

jjestrada2 commented Feb 3, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jjestrada2 @garydgregory