0

I have a list of books. Some of these books are the same however they are not duplicates as they differ because of a variable called copyNumber. Example. Book1(Title: Spiderman, ISBN: 1111, CopyNo: 1), Book2(Title: Spiderman, ISBN: 1111, CopyNo: 2), Book3(Title: Spiderman, ISBN: 1111, CopyNo: 3), Book4(Title: Alice in wonderland, ISBN: 2222, CopyNo: 1), Book5(Title: Alice in wonderland, ISBN: 2222, CopyNo: 2). So my goal is to list all these books in order of their copy numbers, find the book (and its copies) via its ISBN number, then delete the last copy. So if i was to call deleteBook("1111"); for the example above i would delete the 3rd copy of spiderman. If i was to call deleteBook("2222"); I would delete the second copy of Alice in wonderland

public void deleteBook(String isbn){
    Collections.sort(books, new OrderBooksByCopyNumber());
    Book book = books.stream().filter((b) -> (b.getISBNNumber().equals(isbn))).findFirst().get();
    books.remove(book);

 }

The code above is close to what i want how ever i don't want findFirst(), I want to do the equivalent of findLast(). Below is my comparator if it's of any relevance. Thanks to anyone who can help!

private class OrderBooksByCopyNumber implements Comparator<Book>{

    @Override
    public int compare(Book o1, Book o2) {
        return o1.getCopyNumber() <  o2.getCopyNumber() ? -1 : o1.getCopyNumber() == o2.getCopyNumber() ? 0 : 1;
    }

}
BackSlash
  • 21,927
  • 22
  • 96
  • 136
Tom
  • 81
  • 1
  • 6
  • Reverse your comparator so that it orders from last to first. – RealSkeptic Jan 15 '19 at 11:24
  • sort the list and traverse the list, if next book is different, set current copyNumber to 1. and then delete the duplicates. This is one way of doing it. I'm sure there can be another better way to do it – Akshay Batra Jan 15 '19 at 11:28
  • 2
    Possible duplicate of [Get last element of Stream/List in a one-liner](https://stackoverflow.com/questions/21426843/get-last-element-of-stream-list-in-a-one-liner) – Ricola Jan 15 '19 at 11:28
  • use reverse ordering for faster access since you are looking for the last object, I will submit an answer soon, but g2g now for an hour – gkhaos Jan 15 '19 at 12:03

3 Answers3

0

You could use stream.skip() passing in the number of books with a certain ISBN -1 then do a findFirst().get();

Documented here -> https://docs.oracle.com/javase/8/docs/api/java/util/stream/Stream.html

Pete
  • 205
  • 2
  • 14
  • But the number is not known, unless you go and count them first, which means you do another iteration of the collection instead of just filtering. – RealSkeptic Jan 15 '19 at 11:51
0

First,sort books with the special ISBN.Second,find the Book with max CopyNo.Third,delte the Book

public static void deleteBook(String isbn, List<Book> books) {
    Book book = books.stream()
        .filter((b) -> (b.getISBN().equals(isbn)))
        .sorted(Comparator.comparingInt(Book::getCopyNo))
        .reduce((book1, book2) -> book2).orElse(null);
    books.remove(book);
}

Thanks @RealSkeptic sort in reverse order and first first is more direct

public static void deleteBook(String isbn, List<Book> books) {
    Book book = books.stream()
        .filter((b) -> (b.getISBN().equals(isbn)))
        .sorted((o1, o2) -> o2.getCopyNo()- o1.getCopyNo())
        .reduce((book1, book2) -> book2).orElse(null);
    books.remove(book);
}
TongChen
  • 1,414
  • 1
  • 11
  • 21
0

There are several problems with your code.

First, there is no reason to write your own comparator when all you want is to compare the copy numbers. The comparator you wrote is equivalent to Comparator.comparingInt(Book::getCopyNumber).

Second, if you are using the get method of an Optional, you are probably doing something wrong. In this case, you are not considering the case when the Optional is empty - when no books match the given ISDN. Instead, you should be doing something like

...findFirst().ifPresent( book -> books.remove(book) );

Third, you are sorting the collection. This leaves it sorted by book number as a side effect - which is probably not what you intended. Also, it takes O(N log N) time to sort it - when all you need is a partial maximum which shouldn't require more than O(N).

What you should do instead is:

public void deleteBook(String isbn){
    books.stream()
         .filter((b) -> (b.getISBNNumber().equals(isbn)))
         .max(Comparator.comparingInt(Book::getCopyNumber))
         .ifPresent(book->books.remove(book));
}

Note that you are still traversing the list twice, because the remove method also has to iterate the book collection one by one to find the matching book to delete. It may be more efficient to forget the stream and just work with a good old ListIterator.

int maxCopyNumber = -1;
int bookIndex = -1;

ListIterator iter = books.listIterator();
while ( iter.hasNext() ) {
   Book book = iter.next();
   if ( book.getISBNNumber().equals(isbn) && book.getCopyNumber() > maxCopyNumber ) {
       maxCopyNumber = book.getCopyNumber();
       bookIndex = iter.previousIndex();
   }
}
if ( bookIndex >= 0 ) {
   books.remove(bookIndex);
}

If your list is an ArrayList or similar, this will prevent traversing it twice as the remove is performed in O(1).

Finally, consider using a different data structure altogether! For example, a Map<String,List<Book>>, where the key is the ISDN, and each entry is the list of books, ordered by copy number. This will allow you to reach the list directly, and just remove the last element. (If the list becomes empty, delete the entry altogether, or check it's not empty to begin with).

RealSkeptic
  • 33,993
  • 7
  • 53
  • 79