[
https://issues.apache.org/jira/browse/SOLR-14474?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17105555#comment-17105555
]
David Smiley commented on SOLR-14474:
-------------------------------------
You speak of "a zillion new classes" but I think you meant new *source files*
(the classes already exist, we're just moving them), unless I'm
misunderstanding you. Secondly, clearly a zillion is made up but I don't think
we're really talking about so many source files; like maybe 10.
RE DocValuesAcc: I think these classes should be inside DocValuesAcc because
they are subclasses of DocValuesAcc; there's a tight coupling. Additionally,
you might consider renaming them to remove the redundant suffixes of "DVAcc"
because references to these classes will need to refer to the containing class
already.
RE StatsInfo: I agree it should be its own standalone class because it is not
tightly coupled to StatsComponent. *If* hypothetically only StatsComponent
created these, then I'd argue the reverse.
RE FloatCmp etc.: Weird... these should be static constant anonymous singleton
instances on the parent interface.
RE ArrayOfNameTypeValueJSONWriter inside JSONResponseWriter.java: should
become inner class of JSONResponseWriter.java because it's only used by it.
RE NaNFloatWriter inside JSONResponseWriter.java: only has two subclasses
externally so I think put as inner class of JSONResponseWriter.
RE FacetRangeProcessor: move to its own file; it's huge.
RE FacetParser: move to its own file with it's sub-parsers
----
My suggested guideline for how to refactor a class that is both top-level (not
inside another class), package-private, and that is NOT in its own source file
– a bit of a problem for compilers. The outcome is either to move to its own
source file or to move it inside the scope of the public class it's currently
located near.
* Is this class only referenced by the source file it's currently in? Easy;
move it inside the public class there.
* Is the class tightly associated with the public class in the same source
file? _Probably_ move it inside that public class.
* Is it referenced by >= 5 other source files? _Probably_ move it to its own
source file.
* Is it really long (200+ lines of code)? _Probably_ move it to its own
source file.
* Otherwise, no guidance. Develop your own opinion if possible and ask for
peer-review.
> Fix auxilliary class warnings in Solr core
> ------------------------------------------
>
> Key: SOLR-14474
> URL: https://issues.apache.org/jira/browse/SOLR-14474
> Project: Solr
> Issue Type: Sub-task
> Reporter: Erick Erickson
> Assignee: Erick Erickson
> Priority: Major
>
> We have quite a number of situations where multiple classes are declared in a
> single source file, which is a poor practice. I ran across a bunch of these
> in solr/core, and [~mdrob] fixed some of these in SOLR-14426. [~dsmiley]
> looked at those and thought that it would have been better to just move a
> particular class to its own file. And [~uschindler] do you have any comments?
> I have a fork with a _bunch_ of changes to get warnings out that include
> moving more than a few classes into static inner classes, including the one
> Mike did. I do NOT intend to commit this, it's too big/sprawling, but it does
> serve to show a variety of situations. See:
> https://github.com/ErickErickson/lucene-solr/tree/jira/SOLR-10810 for how
> ugly it all looks. I intend to break this wodge down into smaller tasks and
> start over now that I have a clue as to the scope. And do ignore the generics
> changes as well as the consequences of upgrading apache commons CLI, those
> need to be their own JIRA.
> What I'd like to do is agree on some guidelines for when to move classes to
> their own file and when to move them to static inner classes.
> Some things I saw, reference the fork for the changes (again, I won't check
> that in).
> 1> DocValuesAcc has no fewer than 9 classes that could be moved inside the
> main class. But they all become "static abstract". And take
> "DoubleSortedNumericDVAcc" in that class, It gets extended over in 4 other
> files. How would all that get resolved? How many of them would people
> recommend moving into their own files? Do we want to proliferate all those?
> And so on with all the other plethora of classes in
> org.apache.solr.search.facet.
> This is particularly thorny because the choices would be about a zillion new
> classes or about a zillion edits.
> Does the idea of abstract .vs. concrete classes make any difference? IOW, if
> we change an abstract class to a nested class, then maybe we just have to
> change the class(es) that extend it?
> 2> StatsComponent.StatsInfo probably should be its own file?
> 3> FloatCmp, LongCmp, DoubleCmp all declare classes with "Comp" rather than
> "Cmp". Those files should just be renamed.
> 4> JSONResponseWriter. ???
> 5> FacetRangeProcessor seems like it needs its own class
> 6> FacetRequestSorted seems like it needs its own class
> 7> FacetModule
> So what I'd like going forward is to agree on some guidelines to resolve
> whether to move a class to its own file or make it nested (probably static).
> Not hard-and-fast rules, just something to cut down on the rework due to
> objections.
> And what about backporting to 8x? My suggestion is to backport what's
> easy/doesn't break back-compat in order to make keeping the two branches in
> sync.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
