LinkParsingOperations improvements?

[Murray Altheim <mu...@altheim.com>]

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

Re: LinkParsingOperations improvements?

[Murray Altheim <mu...@altheim.com>]

On 2022/08/05 8:53, Juan Pablo Santos Rodríguez wrote:
> Hi Murray,
> 
> (apologies for the brevity, is really late in here, and won't be
> having too much time next few days..)

Hi Juan Pablo,

No worries, my time is pretty limited lately as well. Thanks very much
for the explanation, I certainly understand refactorings. I myself don't
have any issues with static methods, and on a massive refactoring job I
did a few years ago (something like 30 modules), I ended up creating a
lot of utility classes composed solely of static methods, as the devs
over the years had ended up creating multiple copies of various functions,
etc. and I was essentially collecting all of them into classes categorised
for area of function. Testing wasn't too bad, but of course we already had
unit, integration and system tests for a lot of the features already.

I've been playing with ideas for the graph-based reference manager, have
even been experimenting with some visualisations of wiki links using the
JUNG and JunGraphT libraries:

   http://jung.sourceforge.net/index.html
   https://tomnelson.github.io/jungrapht-visualization/

As something gets closer to reality I'll post links to some of the
generated images. The visualisations are more of a curiosity than something
that would be functional for a working wiki, but are helping me understand
what is actually happening in the GraphReferenceManager as I go along.

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

Re: LinkParsingOperations improvements?

[Juan Pablo Santos Rodríguez <ju...@gmail.com>]

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 <mu...@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
>