Clean way to avoid conditions

Question

I've got a code like this:

 Map<String, String> args = new HashMap<>();

    args.put("-T", "Tom Sawyer");
//        args.put("-I", "1112223334");

    if (args.containsKey("-T")) {
        Book book = libraryService.findBookByTitle(args.get("-T"));
    } else {
        Book book = libraryService.findBookByIsbn(args.get("-I"));                       
    }  

LibraryService:

public class LibraryService {

    private final BookRepository bookRepository = new BookRepository();

    public Book findBookByTitle(String title) {
        return bookRepository.findByTitle(title);
    }

    public Book findBookByIsbn(String isbn) {
        return bookRepository.findByIsbn(isbn);
    }

BookRepository:

public class BookRepository {

 private List<Book> books = new ArrayList<>();

  public Book findByIsbn(String isbn) {
        return books.stream()
            .filter(s -> s.getIsbn().equals(isbn))
            .findFirst()
            .orElseThrow(() -> new RuntimeException(NO_BOOKS_FOUND));
}

    public Book findByTitle(String title) {
        return books.stream()
            .filter(s -> s.getTitle().equals(title))
            .findFirst()
            .orElseThrow(() -> new RuntimeException(NO_BOOKS_FOUND));
}

Is there a clean way to avoid ifs? I want my code to decide whether it has to use argument -I or -T. I handled situations when args don't have any of it, I just simplified code for StackOverflow. I use methods findByTitle and findByIsbn many times in my code, so I'm not sure if another method would suit here.

Answer 1

You can get rid of the if with two combined patterns. I also think the if in the first block is somewhat not on the same level of abstraction as the code around it. The other problem is, that this does not follow the open close principle. Lets say you want another type of Searchparameter like a registernumber -R or whatever. You would have to touch the if statement and if this gets bigger and bigger, you might break something.

so lets see, what a nice code could look like without if

args.put("-T", "Tom Sawyer");
args.put("-I", "1112223334");
Book book = libraryService.findBook(args);

But this obviously does not work like this out of the box. But you can do something like that with the strategy pattern.

IBookFindStrategy {
   Book findBook(string param)
}

class IsbnFindStrategy : IBookFindStrategy {
   Book findBook(string param) {
      // your repocall
   }
}

class NameFindStrategy : IBookFindStrategy {
   Book findBook(string param) {
      // your repocall
   }
}

Now you just have to convert the parameters somewhere else and initialize the correct Strategy in a Factory. The Factory could store the parameters in a Hashmap and would call it with the param -T wich would give you the NameFindStrategy. Something like this

class StrategyFactory {
      Hashtable<String, IBookFindStrategy > strategies;

      public StrategyFactory() {
          strategies = new HashMap<String, IBookFindStrategy >();
          strategies.put("-T", new NameFindStrategy());
          strategies.put("-I", new NameIsbnFindStrategy());
      }

      public IBookFindStrategy GetStrategy(string param) {
          return strategies.get(param);
      }
}

And then finally, your main will look something like this:

StrategyFactory factory = new StrategyFactory();
IBookFindStrategy bookFinder = factory.getStrategy(args);
Book book = bookFinder.findBook(args);

I am not on a dev machine and my java is a little rusty, so i am a little lazy to write everything down, but i hope you get the concept.

Answer 2

As it is, the code seems to be at its simplest and probably best form.

However, you could use a mapping of "book finders" to remove the explicit if blocks. Here's one version that uses suppliers:

Map<String, Function<String, Book>> resolvers = new HashMap<>();
resolvers.put("-T", libraryService::findBookByTitle);
resolvers.put("-I", libraryService::findBookByIsbn);

That can then be used in a short stream of all possible keys:

Book book = Stream.of("-T", "-I").filter(args::containsKey)
                  .findFirst()
                  .map(key -> resolvers.get(key).apply(args.get(key)))
                  .orElse(null);

The above will return null if neither -T nor -I is in the map.

Answer 3

Instead of passing arguments to repository, pass a complete Predicate to it:

public class findOneByPredicate(Predicate<Book> filter) {
        return books.stream()
            .filter(filter)
            .findFirst()
            .orElseThrow(() -> new RuntimeException(NO_BOOKS_FOUND));
}

Then you can call it like:

findOneByPredicte(b -> b.getIsbn().equals("ISBN"));

Answer 4

Your code looks pretty fine, at least to me.

Your if statements don't need removing. They are not duplicated, so keep them there.

On the other hand, I did find some duplicate code in the findBy methods:

public Book findByIsbn(String isbn) {
    return books.stream()
        .filter(s -> s.getIsbn().equals(isbn))
        .findFirst()
        .orElseThrow(() -> new RuntimeException(NO_BOOKS_FOUND));
}

public Book findByTitle(String title) {
    return books.stream()
        .filter(s -> s.getTitle().equals(title))
        .findFirst()
        .orElseThrow(() -> new RuntimeException(NO_BOOKS_FOUND));
}

You can write a new method called findBy:

private Book findBy<T>(Function<Book, T> selector, T value) {
    return books.stream()
        .filter(s -> selector.apply(s).equals(value))
        .findFirst()
        .orElseThrow(() -> new RuntimeException(NO_BOOKS_FOUND));
}

And then have findByIsbn and findByTitle call findBy:

public Book findByIsbn(String isbn) {
    return findBy(Book::getIsbn, isbn);
}

public Book findByTitle(String title) {
    return findBy(Book::getTitle, title);
}