Hi Murray, (apologies for the brevity, is really late in here, and won't be having too much time next few days..)
I extracted the LinkParserOperations to be able to reuse it when developing other parsers; f.ex., you can see it also at the MarkdownParser [#1], and at the LinkTag custom tag [#2] or inside the LinkParser class [#3]. At the time of extracting the operations I went with the Context b/c I didn't knew if I was to need something other than the Engine, so going with the Context made more sense to me. In order to not break other code using it (if ti exists at all), we could overload those methods so they could also receive an Engine? As for the StartingComparator, it's a private static class, so it's only used inside the LinkParserOperations. Probably it was a private static lass inside the JSPWikiMarkupParser, but moved with the LinkParserOperations class when it was extracted. Agree with the linkExists/linkIfExists refactor, don't remember why I didn't that on the first place :-? As for making it static, the objects should be freed as soon as you exit from the block they're created, so garbage collection-wise, they shouldn't impact on the application. Usually I'm not a big fan of static methods, as they're usually harder to test.. not in this case, but usually. Not knowing how would the class take shape, I went the safe way with creating a new object, and then left it that way once it was done: memory footprint should also be neligible, so making it static seemed to me like making only for the sake of it, so I didn't bother too much then. Hope this makes a bit clearer the intent of that class :-) best regards, juan pablo [#1]: https://github.com/apache/jspwiki/blob/2.11.3/jspwiki-markdown/src/main/java/org/apache/wiki/markdown/extensions/jspwikilinks/attributeprovider/ExternalLinkAttributeProviderState.java#L63 [#2]: https://github.com/apache/jspwiki/blob/2.11.3/jspwiki-main/src/main/java/org/apache/wiki/tags/LinkTag.java#L192 [#3]: https://github.com/apache/jspwiki/blob/2.11.3/jspwiki-main/src/main/java/org/apache/wiki/parser/LinkParser.java#L473 On Thu, Aug 4, 2022 at 1:19 PM Murray Altheim <murra...@altheim.com> wrote: > > Hi, > > As I've been digging around in the code related to the ReferenceManager, > I found a curiosity. > > I'm not sure of the history surrounding LinkParsingOperations, but it > seems that we create an instance for every WikiContext, despite the > fact that both of the two methods that use it actually use it just to > obtain the WikiEngine. The other eight methods could be static, and > there's a StartingComparator inner static class that's not actually > used anywhere else in code so it could be removed (so far as I can see). > > The linkIfExists(String) and linkExists(String) methods both do almost > the same thing, so in theory the latter could simply call the former to > obtain its boolean result. > > Since all the places where these two methods are called (all are within > JSPWikiMarkupParser) have access to the WikiEngine it could be passed > in as an argument, and therefore all of the methods of the > LinkParsingOperations could be made static and turn this into a static > utility class. > > There's clearly no urgency for this but it does seem like we could reduce > the number of objects created by adopting the above suggestions. > > Cheers, > > Murray > > ........................................................................... > Murray Altheim <murray18 at altheim dot com> = = === > http://www.altheim.com/murray/ === === > = = === > In the evening > The rice leaves in the garden > Rustle in the autumn wind > That blows through my reed hut. > -- Minamoto no Tsunenobu >
