Showing a list of plugins to filter

Question

How can I refactor this function? I show a list of plugins in JSF, and I should filter them. I added function filterPlugins but I have some questions:

1. Should we simplify block with conditional expression?
2. I reassign global variable "plugins" to introduce my filter function (is it not bad practice?)

load()

@NotNull
@DataModel("plugins")
List<Plugin> plugins;

public void load() {
    plugins = pluginManager.getPlugins();

    plugins = filterPlugins(searchParam);

    SortingUtil.sort(plugins, SortingUtil.SortType.ID_ASC);
    if (plugins.size() > 0) {
        if (plugin != null && plugins.contains(plugin)) {
            selectPlugin(plugin);
        } else {
            selectPlugin(plugins.get(0));
        }
    } else {
        plugin = null;
    }
}

@SuppressWarnings("unchecked")
private List<Plugin> filterPlugins(final String searchParam ){
      return (List)Iterables.filter(plugins, new Predicate<Plugin>() {
        @Override
        public boolean apply(@Nullable Plugin plugin) {
            return  plugin.getName().contains(searchParam)
                    || plugin.getNetworkClasses().contains(searchParam)
                    || plugin.getClassName().contains(searchParam);
        }
    });
}


@Override
public void selectPlugin(Plugin p) {
    plugin = p;        
}

Plugin.java

public class Plugin {

    private static final long serialVersionUID = -8424575107726996696L;

    @NotNull
    private Long id;

    @NotNull
    private String name;

    @NotNull
    private String networkClasses;

    + setters and getters
}

I want to pay your attention this code interacts with JSF page:

<a:support event="onRowClick" action="#{configPlugins.selectPlugin(p)}" reRender="pluginList"/>
<a:commandLink action="#{configPlugins.removePlugin(true)}" reRender="pluginList"/>

Answer 1

Early Return

You can return early to reduce the nesting of your if statements:

if (plugins.isEmpty()) {
    plugin = null;
    return;
}

Also note that I changed plugins.size() > 0 to !plugins.isEmpty(), I think it's more readable.

Plugins Field

Code like this:

plugins = pluginManager.getPlugins(); // assign field

plugins = filterPlugins(searchParam); // method expects plugins field to be assigned, 
                                      // but doesn't work on it but returns instead

Can be a bit confusing. I would change it to:

plugins = filterPlugins(pluginManager.getPlugins(), searchParam);

And then change filterPlugins to:

private List<Plugin> filterPlugins(final List<Plugin> plugins, final String searchParam )

plugin field

I would still like to see the declaration of plugin as well as the selectPlugin method. As mentioned in the comments, it looks a bit like it might be bad design.

Answer 2

A few things that are jumping me here, though it might get a little harsh :(

@NotNull
@DataModel("plugins")
List<Plugin> plugins;

This one is smelly, because you are having database level validation annotations within processing code. It seems that you are mixing responsibilities here. There shouldn't be any necessity to annotate a List<Plugin> in a processing class with @NotNull.

Additionally this allows for multiple plugins. This could also be undesired, you might want to use a Set<Plugin> instead. And according to the Principle of Information Hiding, this should probably be private!

It seems that the @DataModel won't work with a Set, though, so you may as well ignore that part ;)

public void load() {

load is not really a good name. Why would you expose loading? This smells. Instead you should probably make this method private and call it in your constructor. But as you don't show much more code, this could as well just be me.

    plugins = pluginManager.getPlugins();

Why is it a Manager? Manager-classes tend to become god-objects quite fast. I'd try to avoid them wherever possible. Additionally XYManager is somewhat unclear. What exactly does this class do? "It manages stuff" is not a valid answer!

    plugins = filterPlugins(searchParam);

This one looks quite nice actually. Just like you I am a little concerned you reassign a classwide variable.

    SortingUtil.sort(plugins, SortingUtil.SortType.ID_ASC);

This one could be somewhat smelly. It could be easier, if you'd implement Comparable in your Plugin class to provide a so-called "natural ordering", which allows you to drop this to use inbuild methods instead:

    Collections.sort(plugins);

The following code has already been addressed by tim quite throughly, so for now that's it.