[jira] Created: (MAHOUT-190) Make all instance fields private

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.

    [ https://issues.apache.org/jira/browse/MAHOUT-190?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12770517#action_12770517 ]

Ted Dunning commented on MAHOUT-190:
------------------------------------


Totally agree.

In fact, this may be one issue where I am even more extreme in my views than Sean.  Non-private instance variables are VERY rarely a good thing except in throw-away code.

As such, when I see them, I tend to throw away the code!

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

    [ 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.

    [ https://issues.apache.org/jira/browse/MAHOUT-190?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12770588#action_12770588 ]

Sean Owen commented on MAHOUT-190:
----------------------------------

I agree there's a tension between closed-for-modification and open-for-extension. I believe there's more harm in allowing extension without designing for it than the opposite. And in the code so far, I really don't see evidence of explicit design for extension. I see overrideable methods in constructors, etc.

When confronted with a class that could be either closed up to be safe, or designed for extension, I'm all for designing for extension.

But does that ever involve having non-private fields? That's the narrower question I'm asking.

I do think a getter is better than a raw field access, yes. I don't agree that every field should be accessible by default, no, which is why it makes any difference. Something should be accessible on purpose, not by accident, and I don't sense many of the non-public fields in the project now are on purpose.

Replacing it with a getter does little, yes, but it's a step in the right direction? Do we have evidence there is a problem with people wanting to extend and not being able to?

I'd also advance the opposite problem: you have a protected field that the dev really doesn't think of as something used by anyone. He removes or changes it. Oops, just broke two subclassers. Either he reverts and realizes, actually, that's now part of the API, sorry, or finds some workaround (like migrating to getters). Is that a lesser problem?

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

    [ https://issues.apache.org/jira/browse/MAHOUT-190?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12770592#action_12770592 ]

Jake Mannix commented on MAHOUT-190:
------------------------------------

<em>Replacing it with a getter does little, yes, but it's a step in the right direction? </em>

I'm a big fan of getters over *public* instance variables, *always*.  Over protected instance variables?  It's not so bad, much of the time that seems fine, yes, esp. when you're exposing it "on purpose" as you say.  It's the exposing it for the advanced user, who is going to subclass, that I'm wondering about.  Like you say, you're not going to go and add protected getters for everything - only things you think people will need.  But then that comes back to the problem of reading the user's mind...

<em>I'd also advance the opposite problem...</em>

Yeah, this is the only argument I really agree with on this, and the only way to get around it is to yes, accept that anything that someone can get access to, via a protected method in a subclass, is in some sense part of the API.  And for the same reasons why you shouldn't have a millions methods on interfaces (well, some of the same reasons), making an API huge by having *everything* not public be protected by default is a bad idea.

But early on in a project, when it's evolving fast, and you don't know how people are going to use it?  That's not the time to err on the side of "closed for modification", it's the time to assume that every user is moderately advanced (why else would they be using a 0.x product?), and let them try things out, and maybe you find things that really should be designed for extension that you didn't realize before.  Making it harder for people to monkey-with just hampers adoption.  If users do crazy shit in the internals of a core class which they're give protected access to, and then in 0.4 we remove those variables, breaking some implementers, well, they knew what they were getting into by monkeying with internals in a fast moving project, and they'll probably bring it up on the lists, we'll all have a big discussion about whether we need to revert or migrate to getters, and we've all learned something.

When the project is farther along (like Lucene), it's more acceptable to lock things down, but doing it too early just leads to pain for people who want to experiment, in my experience.

Does it *hurt* us to not make this kind of design choice, of enforcing this strict rule, until later on when apis have stabilized and we know who our users are and what they are doing?

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

    [ https://issues.apache.org/jira/browse/MAHOUT-190?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12770621#action_12770621 ]

Sean Owen commented on MAHOUT-190:
----------------------------------

How about this compromise: I think there are several instances that everyone where everyone will agree there's no real case for extension (e.g. test cases). There are some cases where there's a question -- there's some case for extension.

I'll prepare a patch where the 'clear' cases are made private, and otherwise, raw access is replaced with getters/setters. This should be a strict improvement. We retain API openness going forward, just in a slightly more organized way. And the non-controversial cases are 'fixed'.

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

    [ https://issues.apache.org/jira/browse/MAHOUT-190?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12770712#action_12770712 ]

Ted Dunning commented on MAHOUT-190:
------------------------------------


bq. 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.

Surely Sean is merely advocating at the core that we forbid access to instance variables except via getter and setter.  

I agree that too much final annotation is (a) pointless and (b) an unnecessary PITA.  On the other hand, final can make code more comprehensible if used judiciously.

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

    [ https://issues.apache.org/jira/browse/MAHOUT-190?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12770720#action_12770720 ]

Jake Mannix commented on MAHOUT-190:
------------------------------------

bq. Surely Sean is merely advocating at the core that we forbid access to instance variables except via getter and setter.

Yeah, but for the sake of argument, when do we go and create those get/set methods?  We go through now and add them for everything, and by default make these methods protected?  If we don't, we've hidden them completely and they're not available, but if we do, that's a crazy lot of methods added, most of the time when they're not necessary, because nobody should be using them (ie. they are the "clear" cases Sean talks about, of private inner stuff nobody should use - implementation details and so forth).  

So we need to decide which are the "clearly" inner, make those private with no getters, let alone setters, and then go through the "possibly extendable" cases, make those private with protected getter (and setter, if it looks like this is somethign that doesn't need to be final/immutable), and then find the cases of basically public stuff, and make them private with public access methods.

I guess what I'm advocating is that this be taken in a case-by-case basis, instead of a blanket statement of "lets hide everything now, and only add back accessors later as necessary" - yes, I agree with both of you that the basic statement of making them private is fine, as long as you default to imagining the world where an advanced user might think of doing something different with things, and only don't provide at least protected access via accessors when you are really really sure that nobody should access these in any possible subclass.  I personally tend to think that subclasses, since they follow the "is a" qualifier w.r.t. their parent, deserve to be allowed to access a fair amount of their parent.  The problem with this kind of approach, as I keep saying, is that it leads to a proliferation of protected accessor methods.  This isn't doesn't rule out doing this, but it's a slight strike against it in my eyes.

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

    [ https://issues.apache.org/jira/browse/MAHOUT-190?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12770740#action_12770740 ]

Grant Ingersoll commented on MAHOUT-190:
----------------------------------------

-1 on a blanket move to private nor final, especially at this stage of the game.  People need to be able to extend implementations.  We've spent a lot of time in Lucene undoing private/final b/c there is no possible way to predict where the next innovation comes from.

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

    [ https://issues.apache.org/jira/browse/MAHOUT-190?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12770871#action_12770871 ]

Sean Owen commented on MAHOUT-190:
----------------------------------

Yes, I am suggesting we (= I) go through now and create potentially all the getters/setters. It will take me 10 minutes with my IDE.

My personal preference is strongly to design for extension, but failing that, prevent extension if it's not designed for. and a lot of stuff is not yet designed for extension.

I am surprised to hear we'd welcome some dependencies to weave their way into the internal representation of these classes, in ways we aren't tracking. Tens of small subtle bugs come to mind. Oops, now I want to synchronize on some internal object. But I've allowed callers to access it directly, and they are too. Maybe a deadlock occurs. Oops I didn't expect the field to be nulled at this point.

Isn't just opening up the representation just punting on designing for extension? should it not be intentional?

The strong argument for complete extensibility sounds like an argument for no encapsulation, which can't be the idea. There's a line, and I thought encapsulating representation was one of the things farthest from that line. I am sure that's the right thing given my own experience, but we all have different experience, and I'm not pushing this point of view.

One other thing, it's open-source right? this is the very case where the worst-case is just that someone copies/pastes a class. It's not a closed library.

The least change would be to expose absolutely everything through getters/setters. I think you said it Jake -- it's a crazy lot of methods added, most of which are not necessary. But these 'methods' already exist, they're part of the API, in the form of accessible fields. They're in the javadoc. This change is just a 'messenger'.

Why don't I make a patch that does in fact add all the getters/setters, for a look. I think in many cases it will just highlight that the fields aren't going to be useful to any extenders. And we chuck those. And we leave a lot of them. And in 3 versions, can even review them again.

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

    [ https://issues.apache.org/jira/browse/MAHOUT-190?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12771174#action_12771174 ]

Ted Dunning commented on MAHOUT-190:
------------------------------------


I would prefer to make all instance variables private, and then add  getters and setters *only* where used.

Putting getters and setters on everything is not a good idea (in my opinion).

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

     [ https://issues.apache.org/jira/browse/MAHOUT-190?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Sean Owen updated MAHOUT-190:
-----------------------------

    Attachment: MAHOUT-190_1.patch

Partial patch. This addresses public fields. I simply replaced with getters/setters -- but them removed the setter in cases where it didn't appear to make sense (and of course, wasn't used). For instance a few classes accumulate a mean and standard deviation; doesn't really make sense to let a caller set these.

I'm using this to test the waters, see if this seems acceptable or if there concerns about extensibility. There seem to be two strong +1s for this, a +0, and a possible -1 for the patch and I can't proceed with a veto.

> 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
>
>         Attachments: MAHOUT-190_1.patch
>
>
> 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.

    [ https://issues.apache.org/jira/browse/MAHOUT-190?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12772608#action_12772608 ]

Ted Dunning commented on MAHOUT-190:
------------------------------------


I think that this patch is a very nice conservative step very much in the right direction.

+1

> 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
>
>         Attachments: MAHOUT-190_1.patch
>
>
> 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.

     [ https://issues.apache.org/jira/browse/MAHOUT-190?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Sean Owen updated MAHOUT-190:
-----------------------------

    Attachment: MAHOUT-190_2.patch

Patch, round 2 of 3

> 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
>
>         Attachments: MAHOUT-190_1.patch, MAHOUT-190_2.patch
>
>
> 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.

     [ https://issues.apache.org/jira/browse/MAHOUT-190?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Sean Owen updated MAHOUT-190:
-----------------------------

    Attachment: MAHOUT-190_2.patch

OK went ahead and made the rest of the changes I think are appropriate given discussion on the issue. Final part 2 patch attached, looking to submit Monday.

> 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
>
>         Attachments: MAHOUT-190_1.patch, MAHOUT-190_2.patch, MAHOUT-190_2.patch
>
>
> 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.

    [ https://issues.apache.org/jira/browse/MAHOUT-190?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12774638#action_12774638 ]

Ted Dunning commented on MAHOUT-190:
------------------------------------


Is patch-2 reversed?  

I like the reversed changes and can't imagine Sean making the forward changes.

> 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
>
>         Attachments: MAHOUT-190_1.patch, MAHOUT-190_2.patch, MAHOUT-190_2.patch
>
>
> 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.

     [ https://issues.apache.org/jira/browse/MAHOUT-190?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Sean Owen updated MAHOUT-190:
-----------------------------

    Attachment: MAHOUT-190_21.patch

Oops, yes had "reverse patch" checked from a previous action -- didn't realize it was sticky

> 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
>
>         Attachments: MAHOUT-190_1.patch, MAHOUT-190_2.patch, MAHOUT-190_2.patch, MAHOUT-190_21.patch
>
>
> 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.

     [ https://issues.apache.org/jira/browse/MAHOUT-190?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Sean Owen resolved MAHOUT-190.
------------------------------

    Resolution: Fixed

> 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
>
>         Attachments: MAHOUT-190_1.patch, MAHOUT-190_2.patch, MAHOUT-190_2.patch, MAHOUT-190_21.patch
>
>
> 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.