0
\$\begingroup\$

There is repetition here and would appreciate help in improving this code. Suggestions? Here's the framework for it:

Class Author {
  private Integer id;

  public static List<Book> sortBooksAlphabetically( Author a ) {
    Integer id = a.id;
    if (id == null) { return null; }
    List<Book> list = getAllBooksForAuthor( id );
    // do some sorting and return list
  }

  public static List<Book> sortBooksReverseAlphabetically( Integer id ) {
    if (id == null ) { return null; }
    List<Book> list = getAllBooksForMusician( id );
    // do some sorting and return list
  }

  public static List<Book> sortBooksByPopularity( Integer id ) {
    if (id == null ) { return null; }
    List<Book> list = getAllBooksForAuthor( id );
    // do some sorting and return list
  }

  public static List<Book> getAllBooksForAuthor( Integer id ) {
     if (id == null ) throw new NullPointerException();
     ...
     return listOfBooks;
  }
}
\$\endgroup\$
8
  • \$\begingroup\$ qconlondon.com/london-2009/presentation/… ;-) \$\endgroup\$ Jun 3, 2013 at 23:03
  • \$\begingroup\$ Try not to use static classes. When is int going to be null? if id==null then java will already throw a NullPointerException no need to throw another one. \$\endgroup\$ Jun 4, 2013 at 0:09
  • \$\begingroup\$ @RobertSnyder "sneaking" NullPointerExceptions are never a good idea... \$\endgroup\$
    – fge
    Jun 4, 2013 at 0:59
  • \$\begingroup\$ Just one "side remark": you say you want to refactor and optimize the class, fine; but what is this class supposed to do to begin with? \$\endgroup\$
    – fge
    Jun 4, 2013 at 1:09
  • 1
    \$\begingroup\$ @RobertSnyder while I agree with you, I suspect the problem here is much deeper than that. Dubious NPE handling is the least of the OP's concern here :/ \$\endgroup\$
    – fge
    Jun 4, 2013 at 3:03

1 Answer 1

3
\$\begingroup\$

Tentative answer...

First of all, it looks like you didn't include the source of the full Author class. Where is the list of Books for that author, for instance?

Second: you seem to want methods to sort this inner list of books for a given author; why, then, pass the id of the author as an argument? You should have a lookup method for an Author given its id -- but this lookup method should not be a method of one Author instance; at worst, it should be a static method in the Author class. What is your backend like? How do you look up an author by its id eventually?

Third: the JDK has Collections.sort(); and given that you want to have a sorted list of books according to several criteria, you want to write several Comparators for that book. See below for a solution to that.

Fourth, and not the least: you should not operate on the list in the Author class itself, but on a copy of that list.

Given these four points above, here are some code extracts...

First: how to return a List<Book> from an author -- don't return the list itself but at least an unmodifiable version of it:

// in class Author; it is supposed that the list has name "bookList" in the class
public List<Book> getBookList()
{
    return new Collections.unmodifiableList(bookList); // not modifiable!
}

Second: you want book lists sorted in various ways; one solution is to create an enum:

public enum BookSort
{
    BY_NAME (new Comparator<Book>()
    {
        @Override
        public int compare(final Book a, final Book b)
        {
            return a.getName().compareTo(b.getName());
        }
    }),
    // other values

    private final Comparator<Book> comparator;

    BookSort(final Comparator<Book> comparator)
    {
        this.comparator = comparator;
    }

    // Return a sorted copy of books
    public List<Book> sort(final List<Book> list)
    {
        final List<Book> ret = new ArrayList<Book>(list);
        Collections.sort(ret, comparator);
        return ret;
    }
}

Then, in your code:

final List<Book> sorted = BookSort.BY_NAME.sort(original);

But all this code is moot to begin with if you do not provide the full code of both the Author and Book class; along with the existing mechanism to grab instances of these two classes.

\$\endgroup\$
5
  • \$\begingroup\$ +1 @viv you will find all explanations about 'fge' response in the Joshua Bloch's book Effective Java amazon.com/Effective-Java-Edition-Joshua-Bloch/dp/0321356683 . If you want to write good and secure code, it is perhaps the only book you have to understand deeply \$\endgroup\$
    – cl-r
    Jun 4, 2013 at 7:00
  • \$\begingroup\$ what elegant solution for the comparator. Last time I did those it wasn't near as nice looking as that. \$\endgroup\$ Jun 4, 2013 at 13:36
  • \$\begingroup\$ Actually, it is even bettter to use an enum here. \$\endgroup\$
    – fge
    Jun 4, 2013 at 13:59
  • \$\begingroup\$ @fge I'm trying my best to understand how a enum would be better. Unless you mean to have just one sort method with a switch in it. I think I would overload the sort method and have the second one pass in a comparator. This way you have a default sort (say by book id) and you can pass in a mock comparator if needed. \$\endgroup\$ Jun 4, 2013 at 14:32
  • \$\begingroup\$ @RobertSnyder that is a design choice, really... I kind of don't like "data" classes which do too much :p Now, if the OP included his full code... \$\endgroup\$
    – fge
    Jun 4, 2013 at 14:35

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