PanelScrollable - a reusable class for rendering a changeable number of panels

This class (PanelScrollable) is part of my view package, in an MVP designed Java Swing system. I am not using any MVP frameworks, it's all custom.

The reason I'm posting this code is that I fear too much of the rendering logic is directly accessing Swing components, making it difficult to unit test; as a result this class has probably had more bugs making it to production than any other single class in the entire application.

What I'd like to do is tease apart the logic that decides what actions should be done on Swing components from the actual calls to Swing methods, but I'm finding that a bit challenging.

First, some custom classes you need to understand the PanelScrollable class. These are used to encapsulate Swing behavior away from the Presenter classes, generally speaking. Provider is from Guice.

public interface Provider {
    T get();
}

abstract class Componentable {
    abstract Component getComponent();
}

public abstract class DataComponentable extends Componentable {
    public abstract void setData(E data);
    public abstract boolean isEnabled();
    public abstract void setEnabled(boolean enabled);
}

And here's PanelScrollable:

```
import static com.google.common.base.Preconditions.checkArgument;

import java.awt.Component;
import java.awt.Dimension;
import java.awt.Rectangle;
import java.util.*;

import javax.swing.*;
import javax.swing.border.EmptyBorder;

import com.google.common.collect.Lists;
import com.google.inject.Provider;

abstract class PanelScrollable extends Componentable {
private static final int STRUT_SIZE = 2;

private JScrollPane outerPane;
private ScrollPanel innerPanel;

private List> panels = Lists.newArrayList();
private List struts = Lists.newArrayList();

private Provider> panelFactory;

PanelScrollable(Provider> panelFactory) {
this.panelFactory = panelFactory;

innerPanel = new ScrollPanel();

outerPane = new JScrollPane(innerPanel);
outerPa

This code is awesomely written. I still have a few really small remarks:

Ordering members:


If openness separates concepts, then vertical density implies close association. So lines of code that are tightly related should appear vertically dense.

- Robert C. Martin, Clean Code, Ch. 5 - Vertical Density (p. 79)

When I skimmed your code from top to bottom (as usually done), I instantly wondered: addPanelAndStrut() - Where does this get called, why is there no parameter and why is it private?

btw. It gets called 5 methods later, and only there... I prefer to have the called method below the calling method, this makes skimming the code much easier, as you don't have to scroll up to see what the call to method xy does.

Reorder your methods a little. Usually you have from top to bottom:

fields, constructors, public methods, private methods, getters and setters

Unneeded parameters:

Your addAndUpdatePanels is only called once, namely from within setData. And It's always called with dirty = false;

Why are you passing in dirty? You can assume that it's not dirty for the purpose of your method. You only need to return whether your method dirties the paint area. The method needn't concern itself with the current state of the paint-area. Remove the parameter and instead just call addAndUpdatePanels(data);

Multiline Conditionals:

This may be only my preference, but I really prefer to assign booleans only once, or rather as seldom as possible.

I'd rewrite your setData as follows:

public void setData(Collection data) {
    boolean dirty = false;

    dirty = addAndUpdatePanels(data) || removeExtraPanels(data.size());
    if(dirty) {
        redrawPanels();
    }
}

Finally:

Very nicely and descriptively named variables and methods make reading your class a joy ;) Keep it up. The ordering of your methods is a little confusing to me. All else equal: Good work!

StackExchange Code Review Q#47486, answer score: 4