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

refactor ClassUtils.getResourceAsStream to simplify the implementation #56

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

refactor ClassUtils.getResourceAsStream to simplify the implementation #56

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 19 additions & 29 deletions velocity-engine-core/src/main/java/org/apache/velocity/util/ClassUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,53 +107,43 @@ public static Object getNewInstance(String clazz)

/**
* Finds a resource with the given name. Checks the Thread Context
* classloader, then uses the System classloader. Should replace all
* calls to <code>Class.getResourceAsString</code> when the resource
* classloader, then uses the System classloader (for compatibility with texen / ant tasks)
* Should replace all calls to <code>Class.getResourceAsString</code> when the resource
* might come from a different classloader. (e.g. a webapp).
* @param claz Class to use when getting the System classloader (used if no Thread
* @param clazz Class to use when getting the System classloader (used if no Thread
* Context classloader available or fails to get resource).
* @param name name of the resource
* @return InputStream for the resource.
*/
public static InputStream getResourceAsStream(Class<?> claz, String name)
public static InputStream getResourceAsStream(Class<?> clazz, String name)
{
InputStream result = null;

/*
* remove leading slash so path will work with classes in a JAR file
*/
while (name.startsWith("/"))
{
name = name.substring(1);
}
name = removeLeadingSlash(name);

ClassLoader classLoader = Thread.currentThread()
.getContextClassLoader();
ClassLoader classLoader = Thread.currentThread().getContextClassLoader();

if (classLoader == null)
if (classLoader != null)
{
classLoader = claz.getClassLoader();
result = classLoader.getResourceAsStream( name );
}
else
{
result= classLoader.getResourceAsStream( name );

/*
* for compatibility with texen / ant tasks, fall back to
* old method when resource is not found.
*/

if (result == null)
{
classLoader = claz.getClassLoader();
if (classLoader != null)
result = classLoader.getResourceAsStream( name );
}
if (result == null)
{
classLoader = clazz.getClassLoader();
if (classLoader != null)
result = classLoader.getResourceAsStream( name );
}

return result;
}

/**
* remove leading slash(es) so path will work with classes in a JAR file
*/
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.

}

/**
Expand Down