11

Please consider this class:

class ClassA{

    private Thing[] things; // stores data

    // stuff omitted

    public Thing[] getThings(){
        return things;
    }

}

This class exposes the array it uses to store data, to any client code interested.

I did this in an app I'm working on. I had a ChordProgression class that stores a sequence of Chords (and does some other things). It had a Chord[] getChords() method that returned the array of chords. When the data structure had to change (from an array to an ArrayList), all client code broke.

This made me think - maybe the following approach is better:

class ClassA{

    private Thing[] things; // stores data

    // stuff omitted

    public Thing[] getThing(int index){
        return things[index];
    }

    public int getDataSize(){
        return things.length;
    }

    public void setThing(int index, Thing thing){
        things[index] = thing;
    }

}

Instead of exposing the data structure itself, all of the operations offered by the data structure are now offered directly by the class enclosing it, using public methods that delegate to the data structure.

When the data structure changes, only these methods have to change - but after they do, all client code still works.

Note that collections more complex than arrays might require the enclosing class to implement even more than three methods just to access the internal data structure.


Is this approach common? What do you think of this? What downsides does it have other? Is it reasonable to have the enclosing class implement at least three public methods just to delegate to the inner data structure?

14

Code like:

   public Thing[] getThings(){
        return things;
    }

Doesn't make much sense, since your access method is doing nothing but directly returning the internal data structure. You might as well just declare Thing[] things to be public. The idea behind an access method is to create an interface that insulates clients from internal changes and precludes them from manipulating the actual data structure except in discreet ways as allowed by the interface. As you discovered when all your client code broke, your access method didn't do that - it's just wasted code. I think a lot of programmers tend to write code like that because they learned somewhere that everything needs to be encapsulated with access methods - but that's for the reasons I explained. Doing it just to "follow form" when the access method isn't serving any purpose is just noise.

I would definitely recommend your proposed solution, which accomplishes some of the most important goals of encapsulation: Giving clients a robust, discreet interface that insulates them from the internal implementation details of your class, and doesn't allow them to touch the internal data structure expect in the ways that you decide are appropriate - "the law of least necessary privilege". If you look at the big popular OOP frameworks, such as the CLR, the STL, the VCL, the pattern you've proposed is widespread, for exactly that reason.

Should you always do that? Not necessarily. For example, if you have helper or friend classes that are essentially a component of your main worker class and are not "front facing", it's not necessary - it's an overkill that's going to add a lot of unnecessary code. And in that case, I wouldn't use an access method at all - it's senseless, as explained. Just declare the data structure in a way that's scoped only to the main class that uses it - most languages support ways of doing that - friend, or declaring it in the same file as the main worker class, etc.

The only downside I can see in your proposal it that it's more work to code (and now you're going to have to re-code your consumer classes - but you have/had to do that anyway.) But that's not really a downside - you need to do it right, and sometimes that takes more work.

One of the things that makes a good programmer good is that they know when the extra work is worth it, and when it isn't. In the long run putting in the extra now will pay off with big dividends in the future - if not on this project, then on others. Learn to code the right way and use your head about it, not just robotically follow prescribed forms.

Note that collections more complex than arrays might require the enclosing class to implement even more than three methods just to access the internal data structure.

If you're exposing an entire data structure through a containing class, IMO you need to think about why that class is encapsulated at all, if it's not simply to provide a safer interface - a "wrapper class". You're saying the containing class does not exist for that purpose - so perhaps there's something not right about your design. Consider breaking up your classes into more discreet modules and layering them.

A class should have one clear and discreet purpose, and provide an interface to support that functionality - no more. You may be trying to bundle things together that don't belong together. When you do that, things will be breaking every time you have to implement a change. The smaller and more discreet your classes are, the easier it is to change things around: Think LEGO.

  • 1
    Thanks for answering. A question: What about if the internal data structure has, maybe, 5 public methods - that all need to be featured by my class' public interface? For example, a Java ArrayList has the following methods: get(index), add(), size(), remove(index), and remove(Object). Using the technique proposed, the class that contains this ArrayList must have five public methods just to delegate to the inner collection. And this class' purpose in the program is most likely not encapsulating this ArrayList, but rather doing something else. The ArrayList is just a detail. [...] – Aviv Cohn May 27 '14 at 23:38
  • The inner data structure is just an ordinary member, that using the technique above - requires it's containing class to feature additional five public methods. In your opinion - is this reasonable? And also - is this common? – Aviv Cohn May 27 '14 at 23:39
  • @Prog - What about if the internal data structure has, maybe, 5 public methods... IMO if you find you need to wrap up an entire helper class inside your main class and expose it that way, you need to re-think that design -your public class is doing too much an/or not presenting the appropriate interface. A class should have a very discreet and clearly defined role, and its interface should support that role and only that role. Think about breaking up and layering your classes. A class should not be a "kitchen sink" that contains all sorts of objects in the name of encapsulation. – Vector May 28 '14 at 5:00
  • If you're exposing an entire data structure through a wrapper class, IMO you need to think about why that class is encapsulated at all if it's not simply to provide a safer interface. You're saying the containing class does not exist for that purpose - so there's something not right about this design. – Vector May 28 '14 at 10:23
  • 1
    @Phoshi - Read-only is the keyword - I can agree with that. But the OP is not talking about read-only. for example remove is not read only. My understanding is the OP wants to make everything public - like in the original code before the proposed change public Thing[] getThings(){return things;} That's what I don't like. – Vector May 28 '14 at 10:34
2

You asked: Should I always encapsulate an internal data structure entirely?

Brief Answer: Yes, most of the time but not always.

Long Answer: I think that classes follow into following categories:

  1. Classes that encapsulate simple data. Example: 2D point. It's easy to create public functions that provide the ability to get/set the X and Y coordinates but you can hide the internal data easily without too much trouble. For such classes, exposing the internal data structure details is uncalled for.

  2. Container classes that encapsulate collections. STL has the classic container classes. I consider std::string and std::wstring among those too. They provide a rich interface to deal with the abstractions but std::vector, std::string, and std::wstring also provide the ability to get access to the raw data. I wouldn't be hasty to call them poorly designed classes. I don't know the justification for these classes exposing their raw data. However, I have, in my work, found it necessary to expose the raw data when dealing with millions of mesh nodes and data on those mesh nodes for performance reasons.

    The important thing about exposing the internal structure of a class is that you have to think long and hard before giving it a green signal. If the interface is internal to a project, it will be expensive to change it in the future but not impossible. If the interface is external to the project (such as when you are developing a library that will be used by other application developers), it may be impossible to change the interface without losing your clients.

  3. Classes that are mostly functional in nature. Examples: std::istream, std::ostream, iterators of the STL containers. It's outright foolish to expose the internal details of these classes.

  4. Hybrid classes. These are classes that encapsulate some data structure but also provide algorithmic functionality. Personally, I think these are a result of poorly thought design. However, if you find them, you have to decide whether it makes sense to expose their internal data on a case by case basis.

In conclusion: The only time I have found it necessary to expose the internal data structure of a class is when it became a performance bottleneck.

  • I think the most important reason that STL exposes their internal data is compatibility with all the functions that expect pointers, which are a lot. – Siyuan Ren Jun 22 '14 at 12:32
0

Instead of returning the raw data directly, try something like this

class ClassA {
  private Things[] things;
  ...
  public Things[] asArray() { return things; }
  public List<Thing> asList() { ... }
  ...
}

So, you are essentially providing a custom collection that presents whatever face to the world is desired. In your new implementation then,

class ClassA {
  private List<Thing> things;
  ...
  public Things[] asArray() { return things.asArray(); }
  public List<Thing> asList() { return things; }
  ...
}

Now you have the proper encapsulation, hide the implementation details, and provide backwards compatibility (at a cost).

  • Clever idea for backwards compatability. But: Now you have the proper encapsulation, hide the implementation details - not really. Clients still have to deal with the nuances of List. Access methods that simply return data members, even with a cast to make things more robust, isn't really good encapsulation IMO. The worker class should be handling all that, not the client. The "dumber" a client has to be, the more robust it will be. As an aside, I'm not sure you answered the question... – Vector May 27 '14 at 2:42
  • 1
    @Vector - you are correct. The data structure returned is still mutable and side-effects will kill the information hiding. – BobDalgleish May 27 '14 at 2:57
  • The data structure returned is still mutable and side-effects will kill the information hiding - yes,that too - it's dangerous. I was simply thinking in terms of what is required from the client, which was the focus of the question. – Vector May 27 '14 at 3:41
  • @BobDalgleish: why not return a copy of the original collection? – Giorgio May 27 '14 at 5:43
  • 1
    @BobDalgleish: Unless there are good performance reasons, I would consider returning a reference to internal data structures to allow its users to change them as a very bad design decision. The internal state of an object should only be changed through appropriate public methods. – Giorgio May 27 '14 at 21:16
0

You should use interfaces for those things. Wouldn't help in your case, since Java's array don't implement those interfaces, but you should do it from now on:

class ClassA{

    public ClassA(){
        things = new ArrayList<Thing>();
    }

    private List<Thing> things; // stores data

    // stuff omitted

    public List<Thing> getThings(){
        return things;
    }

}

That way you can change ArrayList to LinkedList or anything else, and you won't break any code since all Java collections(beside arrays) that have (pseudo?) random access will probably implement List.

You can also use Collection, that offer less methods than List but can support collections without random access, or Iterable that can even support streams but doesn't offer much in term of access methods...

  • -1 - poor compromise and not particularly safe IMO: You are exposing the internal data structure to the client, just masking it and hoping for the best because "Java collections... will probably implement List." If your solution was truly polymorphic/inheritance based - that all collections invariably implement List as derived classes, it would make more sense, but just "hoping for the best" is not a good idea. "A good programmer looks both ways on a one way street". – Vector May 27 '14 at 2:33
  • @Vector Yes, I'm assuming future Java collections will implement List(or Collection, or at least Iterable). That's the whole point of these interfaces, and it's a shame that Java Arrays don't implement them, but they are official interface for collections in Java so it's not that far fetched to assume any Java collection will implement them - unless that collection is older than List, and in that case it's very easy to wrap it with AbstractList. – Idan Arye May 27 '14 at 13:24
  • You're saying that your assumption is virtually guaranteed to hold true, so OK - I'm will remove the down-vote (when I'm allowed to ) because you were decent enough to explain, and I'm not a Java guy except by osmosis. Still, I don't support this idea of exposing the internal data structure, regardless of how it's done, and you haven't directly answered the OP's question, which is really about encapsulation. i.e. limiting access to the internal data structure. – Vector May 27 '14 at 17:27
  • 1
    @Vector Yes, users can cast the List into ArrayList, but it's not like the implementation can be 100% protected - you can always use reflection to access private fields. Doing so is frowned upon, but casting is also frowned upon(not as much though). The point of encapsulation is not prevent malicious hacking - rather, it's to prevent the users from depending on implementation details you might want to change. Using the List interface does exactly that - users of the class can depend on the List interface instead of the concrete ArrayList class that might change. – Idan Arye May 27 '14 at 17:47
  • you can always use reflection to access private fields certainly-if someone wants to write bad code and subvert a design, they can do so. rather, it's to prevent the users... - that's one reason for encapsulation. Another is to ensure the integrity and consistency of the internal state of your class. The problem is not "malicious hacking", but poor organization that leads to nasty bugs. "Law of least necessary privilege"-give the consumer of your class only what's mandatory - no more. If it's mandatory that you make an entire internal data structure public, you've got a design problem. – Vector May 28 '14 at 5:17
-2

This is pretty common to hide your internal data structure from outside world. Sometime it is overkill specially in DTO. I recommend this for domain model. If at all its required to expose, return the immutable copy. Along with this i suggest creating an interface having these methods like get , set , remove etc.

  • 1
    this doesn't seem to offer anything substantial over 3 prior answers – gnat May 28 '14 at 18:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.