RXJava operate on exiting List instead of creating new

Question

I'm dipping my toes in to RXJava for the first time. The function below works just fine but I can't help thinking it can be improved. For instance I'm sure that I'm unnecessarily creating a new List<Dish> sortedDishes when I could be performing operations on the exiting List<Dish> dishes that is passed in, I just don't know how yet.

public void sortDishes(int categoryId, List<Dish> dishes) {
    final List<Dish> sortedDishes = new ArrayList<>();
    final Integer[] subCategories = dbInteractor.getSubCategories(categoryId);
    final ArrayList<Integer> cats = new ArrayList<>();
    cats.add(categoryId);
    if (subCategories != null) {
        cats.addAll(Arrays.asList(subCategories));
    }

    Observable.from(dishes)
            .filter(new Func1<Dish, Boolean>() {
                @Override
                public Boolean call(Dish dish) {
                    Integer[] categories = categoryHelper.getCategorys(dish.getCategories());
                    for (Integer category : categories) {
                        if (cats.contains(category)) {
                            return true;
                        }
                    }
                    return false;
                }
            })
            .subscribe(new Subscriber<Dish>() {
                @Override
                public void onCompleted() {

                }

                @Override
                public void onError(Throwable e) {

                }

                @Override
                public void onNext(Dish dish) {
                    sortedDishes.add(dish);
                }
            });
}

Hi, welcome to Code Review. I hope you receive great answers! — Tunaki
– Tunaki, Commented Apr 18, 2016 at 21:16

Community · Accepted Answer · 2020-06-10 13:24:26Z

You're just abusing the library here. What you're doing can be done significantly more succinctly in standard java API:

public void sortDishes(int categoryId, List<Dish> dishes) {
    final Set<Integer> categories = new HashSet<Integer>();
    categories.add(categoryId);
    final Integer[] subCategories = dbInteractor.getSubCategories(categoryId);
    if (subCategories != null) {
        categories.addAll(Arrays.asList(subCategories));
    }

    final List<Dish> sortedDishes = dishes.stream()
        .filter(d -> categoryHelper.getCategories(d.getCategories())
                         .stream().anyMatch(categories::contains))
        .sort()
        .collect(Collectors.toList());
}

Note that this corrects a typo in getCategorys.

Further notes

Your dbInteractor's API could use a rework. I generally advocate against returning null from APIs that return a collection of elements, whether that's an Array or an actual Collection subclass.

I strongly suggest you consider returning new Integer[0] or even Collections.emptyList() because this will greatly simplify calling code by guaranteeing you're not returning null.

I also don't understand why the method is typed void. Either you actually modify the List you've received or you return a new List.

Thanks for the comments @Vogel612. I should say that I'm doing this on Android and as such don't have access to Streams. My bad for not tagging the question properly. I take on board your advice for returning empty collections instead of null, it is good thanks. The method I posted here was edited to remove some code which was not relevant to the question and operated on the resulting Observable. I included the lines at the top of the method to help describe what data I was operating on . The original question was about RXJava so I wasn't concerned with the rest making sense. Again, thanks. — Ivan Wooll
– Ivan Wooll, Commented Jul 24, 2016 at 11:41

Stack Exchange Network

RXJava operate on exiting List instead of creating new

1 Answer 1

Further notes

You must log in to answer this question.

Hot Network Questions

RXJava operate on exiting List instead of creating new

1 Answer 1

Further notes

You must log in to answer this question.

Related

Hot Network Questions