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

refactor ClassUtils.getResourceAsStream to simplify the implementation #56

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

Conversation

verhasi
Copy link

@verhasi verhasi commented Nov 15, 2024

No functional modification applied just simplified the implementation to make it more readable.

*/
private static String removeLeadingSlash(String name)
{
return name.replaceFirst("^/*", "");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the benefit to move a single line to method which is used only once?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moveover, this requires compiling a regexp every single time.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The trigger for moving to a separate method was the smelling inline comment (that is not a Javadoc). At first, it was three lines of code and after moving I refactored it to an oneliner. I still see it as better readable as the method has a meaningful name.
Regarding the performance. The starting point was a cycle that created a new String object for each starting slash. Depending on the number of starting slashes the replaceFirst is an enhancement. On the other hand, I agree with you this could be split into a static compiled pattern and a matching part.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't startsWith and substring cheap enough?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can go ahead with startsWith and substring. I propose the following:

    /**
     * remove leading slash(es) so path will work with classes in a JAR file
     */
    private static String removeLeadingSlash(String name)
    {
        int i = 0;
        while (name.startsWith("/",i)) i++;
        return i>0 ? name.substring(i) : name;
    }

It performs the same in case of 0 or 1 slash, but I am sure it is faster in case the number of slashes is greater than 1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants