HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

RXJava operate on exiting List instead of creating new

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
newrxjavacreatingoperateinsteadlistexiting

Problem

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 sortedDishes when I could be performing operations on the exiting List dishes that is passed in, I just don't know how yet.

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

    Observable.from(dishes)
            .filter(new Func1() {
                @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() {
                @Override
                public void onCompleted() {

                }

                @Override
                public void onError(Throwable e) {

                }

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

Solution

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 dishes) {
    final Set categories = new HashSet();
    categories.add(categoryId);
    final Integer[] subCategories = dbInteractor.getSubCategories(categoryId);
    if (subCategories != null) {
        categories.addAll(Arrays.asList(subCategories));
    }

    final List 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.

Code Snippets

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());
}

Context

StackExchange Code Review Q#126055, answer score: 2

Revisions (0)

No revisions yet.