[ 
https://issues.apache.org/jira/browse/LUCENE-4659?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13545363#comment-13545363
 ] 

Gilad Barkai commented on LUCENE-4659:
--------------------------------------

Patch looks really good.

I'm not concerned about the new objects for subPath. Actually, since the 
{{HashMap}} s are now against {{CategoryPath}} and it's not constantly being 
translated to/from {{String}} I'm looking forward a better performance than 
before.

Nice job, and a nasty one that must have been.. 
Chapeau à lui!

A few (minor) comments:
* {{copyFullPath()}} - {{numCharsCopied}} is redundant? The return value could 
have been {{(idx + component[upto].length() - start)}} 
* {{equals()}} line 143 - perhaps use only one index rather than both index 
{{j}} and {{i}} ? Also, CPs are more likely to be different at the end, than in 
the start (e.g further away from the root than the dimension) - perhaps iterate 
in reverse (up the tree)?
                
> Cleanup CategoryPath
> --------------------
>
>                 Key: LUCENE-4659
>                 URL: https://issues.apache.org/jira/browse/LUCENE-4659
>             Project: Lucene - Core
>          Issue Type: Improvement
>          Components: modules/facet
>            Reporter: Shai Erera
>            Assignee: Shai Erera
>         Attachments: LUCENE-4659.patch, LUCENE-4659.patch
>
>
> CategoryPath is supposed to be a simple object which holds a category path's 
> components, and offers some utility methods that can be used during indexing 
> and search.
> Currently, it exposes lots of methods which aren't used, unless by tests - I 
> want to get rid of them. Also, the internal implementation manages 3 char[] 
> for holding the path components, while I think it would have been simpler if 
> it maintained a String[]. I'd like to explore that option too (the input is 
> anyway String, so why copy char[]?).
> Ultimately, I'd like CategoryPath to be immutable. I was able to get rid most 
> of the mutable methods. The ones that remain will probably go away when I 
> move from char[] to String[]. Immuntability is important because in various 
> places in the code we convert a CategoryPath back and forth to String, with 
> TODOs to stop doing that if CP was immutable.
> Will attach a patch that covers the first step - get rid of unneeded methods 
> and beginning to make it immutable.
> Perhaps this can be done in multiple commits?

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org

Reply via email to