Is calling .sort() in the constructor a violation of the guideline that a constructor shouldn't do work?

Question

Suppose I have the following List to hold a list of fruits.

Example:

def fruits = ["Apple", "Orange", "Grapes"]
def fruitsBowl = ["Apple", "Grapes", "Orange"]

// Will print false
println(fruits.equals(fruitBowl))

Only after I call .sort() them will both collection be equal.

fruits.sort()
fruitBowl.sort()

// Will print true
println(fruits.equals(fruitBowl))

Suppose if I had a Book class, obviously a Book class will have a list of authors. For this example, the Authors class has implemented the Comparator.

class Book {

    private String title;
    private List<Author> authors;

    Book(title, authors){
        // code to initialize left out
        authors.sort()
    }

My main concern is the equals() method. If two books are compared, even if the book objects are the same, but list of authors is unsorted, it will return false.

If I were to call .sort() the same way I call sort on fruits and fruitsBowl on the collection holding the list of authors in the Book constructor, is it bad practice?

If it's bad practice, what should I do to ensure that the equals method works?

Update:

As per the comments suppose if I used a Set, it makes much more sense, because if the books are written by a list of authors, each name will only appear once, so is doing this also considered bad practice?

class Book {

    private String title;
    private Set authorsSet;

    Book(title, authors){
       authorsSet = authors.toSet() 
    }

A Set is a collection of unique elements and the equality test will work properly, regardless of order.

Answer 1

Is calling .sort() in the constructor a violation of the guideline that a constructor shouldn't do work?

Not if .sort() must be called in order to construct a valid object.

What is a valid object, you say? A valid object is one that fulfills the constraints you've imposed on it. What are those constraints? Whatever you say they are.

var sortedList = new SortedList(unsortedList);

Naturally, there are practical limits. Yesterday, someone asked a question about the Spring Framework AnnotationConfigApplicationContext class. Instantiating this class takes 15 seconds in his application, because it registers and makes available to the application a couple hundred forms. Well, 15 seconds is a pretty damn long time in computing terms, but I have applications on my computer right now that take that long to load.

This guy's problem is not the amount of work that the constructor is performing; it is that he's doing that work on every page load of a web application.

"Constructors should not do real work" is a red herring. Focus more on appropriate use.

Answer 2

The misunderstanding is that you don't have a List of authors (unless author order is important, such as in academia - but then you'd never even think of calling sort on that List), but rather you have a Set or Collection of authors.

Furthermore, the List contract specifies things that imply that it may be an unsorted list at any time. The .add() method adds to the end always.

As this is dealing with groovy, it is important to note the documentation on == and .equals() - http://groovy-lang.org/style-guide.html#_equals_and_code_code

Lists are equal if they contain the same items in the same order. - http://docs.groovy-lang.org/latest/html/groovy-jdk/java/util/List.html#equals(java.util.List)

Sets are equal if they contain the same items. http://docs.groovy-lang.org/latest/html/groovy-jdk/java/util/Set.html#equals(java.util.Set)

If you don't care about the order of the items in the Collection, don't use a List. If you are sorting the items in a List to get List equality, you don't care about the order.

Answer 3

Yes it's bad practice.

sort alters the List you passed in, thanks to Java's confusing parameters by ref/value semantics.

You don't really want to get side effects from methods (or constructors)

In addition,

1. It's good to avoid work in constructors, say I'm instantiating a million books to list by title.

2. It's (probably) a view concern and doesn't need to be in the object

I would override the equality operator on Book to provide your custom comparison. Maybe just compare the ISBN number to avoid sorting the list. Or compare a sorted copy of the list.

Update in response to edit:

List.toSet() will remove duplicate authors. Just pass in a Set and, as a bonus, avoid work in the constructor!