Recently i've been studying the principles of object oriented programming in Java and I have heard that making a getter return a whole collection (like a List) of objects is generally a bad practice.

So I tried a different approach, like this one:

public class Myclass {

    private List<MyObject> myObjects;

    //other code...

    public List<MyObject> getMyObjects(Predicate<MyObject> filter) {

        return myObjects
            .stream()
            .filter(filter)
            .Collect(Collectors.toList());
    }
}

Basically my idea was to pass to the getter a predicate like :

myObject -> myObject.getFeatureX().equals(specifiedTrait)

This way the method would return only the objects that match a specified trait.

Is doing things this way a bad idea? why?

CodePudding user response：

The principle is not that the user may or may not want to get all the elements of the list. The reason why it is considered a bad idea has to do with giving access to users to a data structure that could potentially mutate unbeknownst to you (the provider). Instead, the principle suggests that you should provide either a defensive copy of it. In fact, many argue that an unmodifiable copy should be provided. If the client needs a modifiable list, then the client can make its own copy.

How to make an unmodifiable copy of a collection?

This is one of the easiest ways to create an unmodifiable list

List<MyObject> myList = new ArrayList<>();
...
List<MyObject> clone = Collections.unmodifiableList(myList);

There is nothing wrong with your approach. Many people don't feel as strongly to code for immutability. But, if you do want to observe this principle, you can do this with your method.

public List<MyObject> getMyObjects(Predicate<MyObject> filter) {
    List<MyObject> temp = myObjects
        .stream()
        .filter(filter)
        .Collect(Collectors.toList());

    return Collections.unmodifiableList(temp);
}

This is a much better approach.

Why return an unmodifiable list?

The main benefit is thread-safety. But even if you don't have a multi-threaded application, you may not want the contents of your list to be modified by third-parties because it might affect something your application may need to do at some point in the future.

CodePudding user response：

I have heard that making a getter return a whole collection (like a List) of objects is generally a bad practice.

The advice that you probably saw is that a getter (or any other method) that returns a list that is a component of a target object is breaking abstraction.

• If the list returned is mutable, then the caller can modify the list without the target object mediating the changes. This is undesirable from an abstraction viewpoint, and also from the viewpoint of correctness.

• If the list returned is immutable, the caller may still be able to observe changes made to the target object's state. This may be undesirable¹.

The standard advice is to either return a copy of the target object's list, or return an unmodifiable wrapper for the list. There are pros and cons for both advised solutions. And in some circumstances, it is necessary to ignore the advice, and knowingly implement a leaky abstraction anyway.

Your getter is different to a typical getter. You are already creating and returning a copy of the original list.

return myObjects
        .stream()
        .filter(filter)
        .Collect(Collectors.toList());   // <<-- Creates a new list!

Therefore, you don't need to do anything.

Is doing things this way a bad idea? why?

• It seems unnecessary. You are adding functionality to the Myclass API class that looks like it is the concern of the caller.

• If this filtering functionality is widely used by callers, then putting it into the API is avoiding repetitive code in callers ... or the need to create utility methods.

• If this class needs to be thread-safe or needs to be used in a multi-threaded context, then there may be thread-safety or correctness concerns if the caller has to do the filtering.

In short, "good idea" or "bad idea" depends heavily on the context.

^{1 - For example, the caller may not be able to cope with the wrapped list changing as a result of other calls on the original target object. And using Collections.unmodifiableList certainly DOES NOT guarantee thread-safety ... in the face of changes to the wrapped list.}