verhasi commented on code in PR #56: URL: https://github.com/apache/velocity-engine/pull/56#discussion_r1850391300
########## velocity-engine-core/src/main/java/org/apache/velocity/util/ClassUtils.java: ########## @@ -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("^/*", ""); Review Comment: 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org For additional commands, e-mail: dev-h...@velocity.apache.org