0

I'm learning some tricks from Java 8. I created a simple list:

 private void createData() {
        bottles.add(new Whiskey("Jack Daniels", "PL"));
        bottles.add(new Whiskey("Balentains", "PL"));
        bottles.add(new Whiskey("Balentains", "EN"));
        bottles.add(new Whiskey("Balentains", "EN"));
        bottles.add(new Whiskey("Balentains", "GR"));
        bottles.add(new Whiskey("Balentains", "PL"));
        bottles.add(new Whiskey("Balentains", "GR"));
    }

And now I would like to get items from this list by few things. If user give a parameter origin, I want to filter this list by origins, but when he gave wrong origin then he should get empty list and when he won't give origin parameter then he should get whole list.

I have a method which filtering items in list:

 private Optional<List<Whiskey>> getWhiskeyFromCountry(String origin) {
        final List<Whiskey> whiskies = bottles.stream()
                .filter(b -> b.getOrigin().equals(origin))
                .collect(Collectors.toList());

        return whiskies.isEmpty() ? Optional.empty() : Optional.of(whiskies);
    }

And also a main method which get parameter (or not) and response with result:

private void getAll(RoutingContext routingContext) {
        Optional<String> origin = Optional.ofNullable(routingContext.request().params().get("filter"));
        List<Whiskey> result = getWhiskeyFromCountry(origin.orElse("")).orElse(Collections.EMPTY_LIST);

        routingContext.response()
                .putHeader("content-type", "application/json; charset=utf-8")
                .end(Json.encodePrettily(origin.isPresent() ? result : bottles));
    }

The problem is that I still use if statemant in last line and I do not want do this. I would like to change this code into clear and functional. I tried to do some magic with Optionals but in the end I got this one and I think it can be do better and simpler. Could you help me? Or maybe this code is good and I do not need to change anything? This question is more about clean code.

Developus
  • 1,400
  • 2
  • 14
  • 50
  • 1
    Just return a `List` from the `getWhiskeyFromCountry` method. If the `origin` is wrong, the list will be empty. – Alex R Oct 19 '19 at 19:54
  • Why not simply return the empty list? Should work as well. – Johannes Kuhn Oct 19 '19 at 19:58
  • @AlexR - it is ok, but still I need to check if statement in last line – Developus Oct 19 '19 at 20:01
  • in `getAll` method no need to init `origin` as optional – Hadi J Oct 19 '19 at 20:01
  • 1
    Why are you trying to avoid if statements? AN if statement is nice, clear, fast, and maintainable. Chains of functional code are not, and perform horribly. – Gabe Sechan Oct 19 '19 at 20:01
  • Also, if looking up by origin is something you want to do frequently, a list of whiskeys is the wrong data structure to use. All your searches are O(n). – Gabe Sechan Oct 19 '19 at 20:03
  • @GabeSechan - sometimes, but building whole code with ifs is not good practise – Developus Oct 19 '19 at 20:03
  • @allocer No, there is nothing wrong with usign an if. I'm not sure where you got the idea that if statements should be avoided, but its absolutely wrong. – Gabe Sechan Oct 19 '19 at 20:04
  • @GabeSechan - I did not say that should be avoided. But I would like to do this code in functional style. Also I think that for example building "if ladders" can be change by functional style – Developus Oct 19 '19 at 20:06
  • @allocer Funcitonal has nothing to do with avoiding ifs. Every functional language in existance has if statements. You don't understand what functional is. Functional tries to avoid *state*, not if statements. – Gabe Sechan Oct 19 '19 at 20:07
  • @GabeSechan - yes, but you can also minimalize if statements in the code to get more readability. For example if you want to filter list you use streams which remove a lot of ifs from the code. – Developus Oct 19 '19 at 20:09
  • 1
    @allocer No, if you're trying to use streams to minimize if statements, you're not understanding streams, funcitonal programming, and making less readable code. You're replacing a nice, easily readable high performing if with a slow, low performance, less readable function call. This is not something you should be attempting to do. It's poor style and bad programming. – Gabe Sechan Oct 19 '19 at 23:46

1 Answers1

2

You can make this method getWhiskeyFromCountry to accept Optional<String> as parameter

private List<Whiskey> getWhiskeyFromCountry(Optional<String> origin)

and then if Optional is empty return empty list or return list based on filter, If user enters the wrong origin still you will get the empty list

return origin.map(o->bottles.stream()
            .filter(b -> b.getOrigin().equals(o))
            .collect(Collectors.toList())).orElse(Collections.EMPTY_LIST);

Or in the above code you can make small tweak to return List for this method getWhiskeyFromCountry

 private List<Whiskey> getWhiskeyFromCountry(String origin) {

    return bottles.stream()
            .filter(b -> b.getOrigin().equals(origin))
            .collect(Collectors.toList());
  }

And in the main method use Optional.map

Optional<String> origin = Optional.ofNullable(routingContext.request().params().get("filter"));
List<Whiskey> result = origin.map(o->getWhiskeyFromCountry(o))
                             .orElse(bottles);
Ryuzaki L
  • 37,302
  • 12
  • 68
  • 98
  • 6
    Passing `Optional` as parameter is not best practice! – Hadi J Oct 19 '19 at 19:58
  • May i know why @HadiJ ? – Ryuzaki L Oct 19 '19 at 20:00
  • 1
    @Deadpool, Although I disagree by getting `origin` as optional, but for your second approach to avoid double checking i think it's better use this `origin.map(o->getWhiskeyFromCountry(o)) .orElse(bottles);` – Hadi J Oct 19 '19 at 20:43
  • Or maybe something like this `Optional getParam(RoutingContext routingContext){return Optional.ofNullable(routingContext.request().params().get("filter"));}` and then in `getAll` method `getParam(routingContext).map(o->getWhiskeyFromCountry(o)) .orElse(bottles);` – Hadi J Oct 19 '19 at 20:46
  • 2
    For references to what @HadiJ pointed out. [Why should Optional not be used in arguments.](https://stackoverflow.com/questions/31922866/why-should-java-8s-optional-not-be-used-in-arguments) – Naman Oct 20 '19 at 02:14