[
https://issues.apache.org/jira/browse/MAHOUT-190?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12770575#action_12770575
]
Jake Mannix commented on MAHOUT-190:
------------------------------------
Oh lordy... the combinatinon of private and final is a scary one to me. It
means we end up leaning far too much in the direction of "closed for
modification" as compared to "open for extension".
I know you can make the getter protected, if necessary. But in a library such
as this one, in which every extension is of the "expert" level, assuming the
end user isn't going to need to access "private" data in their operation is a
bad one.
The number of times I've needed to do something the implementer didn't expect
is really high. For example, imagine we're talking about the IntDoubleVector,
a sparse impl which has int[] indices, and double[] values, and implements
Vector. Great, it has all the useful methods, like the iterators, and linear
methods like dot and so forth. Now as user, I end up needing to know, for my
application, what the current maximum index (in this vector) is, and the
minimum index. Neither of these are exposed via a Vector API, and in fact,
most likely nobody will put a getIndices() method on the class itself, because
it's "internal". If they're private, then we're screwed - nobody can
efficiently getMaxIndex() without modifying the class itself, because you can't
subclass it to get access. You then say, well the right answer is to give a
read-only getIndices method, except in this case, you've got an array, and if
you've actually lost any real immutability if you were trying to enforce any,
because the contents of the array are modifiable even if the array is final.
Also, what are you going to do, write protected getters for every single
private instance variable, to make sure that subclasses *can* get access to
them if they need to? How is it less magic to see, in the middle of some
subclass, "int[] foo = getIndices();" vs. "int[] foo = this.indices;" ?
I like to think of myself as someone who doesn't write "bad smelling" code, and
yet I find myself, *especially* in apache projects, running into cases where
the class designer assumes that nobody will need to do something with a class,
when it's just that they haven't thought of it yet. When the project in
question is a full-fledged application (Solr, HBase, etc...), I'm fine with
this - it's supposed to be *easy*, and users may be not supposed to dig into
the internals that often. When the project is a *library* (Lucene, Mahout,
Commons-*, etc...), for use by a very wide audience, it can get beyond
annoying.
For project committers, you don't notice the pain of this as much: if you see a
new use for a class, you can just add a method, or decide to make an exception
and change the private instance to a protected one, but end users can't do
this. All they can do is petition to the dev list, submit a patch, and then
try to beg and argue to get the patch committed, by which time they've probably
just forked the code and recompiled with their own patches locally, maybe just
giving up on the system entirely.
Sorry if I sound bitter, but this whole "open/closed" principle can really go
wrong when you go the extreme route of assuming you know what people are going
to do with the code, and assuming that because someone's design "doesn't smell
right" to you means that you should be dictating how they code. It's rather
paternalistic. So I don't give a rat's ass about the performance of getters,
it's much more that the *design choice* of hiding everything, even from
subclasses (don't get me started on making classes final which don't need to be
final!), that I feel pretty strongly about (and having protected getters for
everything leads to a horribly cluttered api and doesn't make sense).
> Make all instance fields private
> --------------------------------
>
> Key: MAHOUT-190
> URL: https://issues.apache.org/jira/browse/MAHOUT-190
> Project: Mahout
> Issue Type: Improvement
> Affects Versions: 0.2
> Reporter: Sean Owen
> Assignee: Sean Owen
> Priority: Minor
> Fix For: 0.3
>
>
> This one may be more controversial but is useful and interesting enough to
> discuss.
> I personally believe instance fields should always be private. I think the
> pro- and con- debate goes like this:
> Making all fields private increases encapsulation. Fields must be made
> explicitly accessible via getters and setters, which is good -- default to
> hiding, rather than exposing. Not-hiding a field amounts to committing it to
> be a part of the API, which is rarely intended. Using getters/setters allows
> read/write access to be independently controlled and even allowed -- allows
> for read-only 'fields'. Getters/setters establish an API independent from the
> representation which is a Good Thing.
> But don't getters and setters slow things down?
> Trivially. JIT compilers will easily inline one-liners. Making fields private
> more readily allows fields to be marked final, and these two factors allow
> for optimizations by (Proguard or) JIT. It could actually speed things up.
> But isn't it messy to write all those dang getters/setters?
> Not really, and not at all if you use an IDE, which I think we all should be.
> But sometimes a class needs to share representation with its subclasses.
> Yes, and it remains possible with package-private / protected getters and
> setters. This is IMHO a rare situation anyway, and, the code is far easier to
> read when fields from a parent don't magically appear, or one doesn't wonder
> about where else a field may be accessed in subclasses. I also feel like
> sometimes making a field more visible is a shortcut enabler to some bad
> design. It usually is a bad smell.
> Thoughts on this narrative. Once again I volunteer to implement the consensus.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
