Best approach to refactor this java snippet

I was told it is not a good style to call potentially costly methods for boolean expressions (getSupercategories()).

private final SuperCategoriesResolver<ProductModel> catResolver = new SuperCategoriesResolver<ProductModel>() {
    @Override
    public Set<CategoryModel> getSuperCategories(final CategoryModel item) {
        return item == null || item.getSupercategories() == null ? Collections.EMPTY_SET
                : new LinkedHashSet<CategoryModel>(
                        item.getSupercategories());
    }
};

As well that getSupercategories() is potentially dangerous since it's backed by a relation attribute which might not be coming from local data members (item is sent as a parameter to a public method in this class and after wards is sent to getSuperCategories() which is overriden in the same class when declaring catResolver).

Is this a better approach to tackle the argument above?

private final SuperCategoriesResolver<ProductModel> catResolver = new SuperCategoriesResolver<ProductModel>() {
    @Override
    public Set<CategoryModel> getSuper开发者_Go百科Categories(final ProductModel item) {
        if (item != null) {
            Set<CategoryModel> superCategories = (Set<CategoryModel>) item
                    .getSupercategories();

            if (superCategories != null)
                return superCategories;
        }

        return Collections.EMPTY_SET;
    }
};

Where I first verify that item is not null. if it is, then a return empy_set if not then I called the costly method and get the collection and just if it is not null return the collection with elements.

Thank u very much for your advice.

It is likely to get more efficient to call getSupercategories() once instead of twice if it does any computation.

Do you need to return a copy of this set? You do in the first example but not the second.

Second approach is indeed faster because there is only one call to the getSupercategories method if item is not null. However, in your second approach, you no longer create a LinkedHashSet instance -- which means it will behave differently (though faster).

This sounds more like performance optimization as opposed to refactoring. Usually when you refactor something, there is a "factor" in there somwhere, that trims the code down by eliminating redundancies.

Nulls are your problem. Can you make a refactoring to push nulls away?

For example, you could refactor your code to make item.getSuperCategories never return null? Or do you need to distinguish between the empty set and null?

Similarly, why are you passing null into this method? If you can eliminate that scenario then the code just becomes a one liner.