10
\$\begingroup\$

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

abstract class Componentable {
    abstract Component getComponent();
}

public abstract class DataComponentable<E> 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<E> extends Componentable {
    private static final int STRUT_SIZE = 2;

    private JScrollPane outerPane;
    private ScrollPanel innerPanel;

    private List<DataComponentable<E>> panels = Lists.newArrayList();
    private List<Component> struts = Lists.newArrayList();

    private Provider<? extends DataComponentable<E>> panelFactory;

    PanelScrollable(Provider<? extends DataComponentable<E>> panelFactory) {
        this.panelFactory = panelFactory;

        innerPanel = new ScrollPanel();

        outerPane = new JScrollPane(innerPanel);
        outerPane.setVerticalScrollBarPolicy(ScrollPaneConstants.VERTICAL_SCROLLBAR_ALWAYS);

        addPanel();
    }

    // In general this is the contract of Componentable, to allow for Swing encapsulation
    @Override
    final Component getComponent() {
        return outerPane;
    }

    private void addPanelAndStrut() {
        synchronized(innerPanel.getTreeLock()) {
            Component newStrut = Box.createVerticalStrut(STRUT_SIZE); 
            struts.add(newStrut);
            innerPanel.add(newStrut);

            addPanel();
        }
    }

    private void addPanel() {
        synchronized(innerPanel.getTreeLock()) {
            DataComponentable<E> newPanel = panelFactory.get();
            innerPanel.add(newPanel.getComponent());
            panels.add(newPanel);
        }
    }

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

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

    private void redrawPanels() {
        innerPanel.validate();
        innerPanel.repaint();
    }

    private boolean addAndUpdatePanels(Collection<E> data, boolean dirty) {
        Iterator<E> it = data.iterator();
        int i = 0;
        while(it.hasNext()) {
            if(notEnoughPanels(i)) {
                addPanelAndStrut();
                dirty = true;
            }
            E next = it.next();
            panels.get(i).setData(next);
            i++;
        }
        return dirty;
    }

    private boolean notEnoughPanels(int i) {
        return i == panels.size();
    }

    private final boolean removeExtraPanels(int index) {
        checkArgument(index > -1, "Cannot remove more panels than exist!");
        if(index >= panels.size()) return false;
        synchronized(innerPanel.getTreeLock()) {
            do {
                removePanelAndStrut();
            } while(index >= panels.size());

            if(index == 0) addPanel();
        }
        return true;
    }

    private void removePanelAndStrut() {
        synchronized(innerPanel.getTreeLock()) {
            DataComponentable<E> removedPanel = panels.remove(panels.size() - 1);
            removedPanel.setEnabled(false);
            innerPanel.remove(removedPanel.getComponent());

            // This is where the bug WAS that made me decide to post this class on code review
            // if setData was called with an empty collection, this code would (previously)
            // call struts.remove(-1)
            if(struts.size() > 0) {
                Component removedStrut = struts.remove(struts.size() - 1);
                innerPanel.remove(removedStrut);
            } else if (panels.size() > 0) {
                throw new IllegalStateException(
                        String.format("There are too many panels (%d) and no struts!",
                                panels.size()));
            }
        }
    }

    private class ScrollPanel extends JPanel implements Scrollable {
        private static final long serialVersionUID = 3823479389541763224L;

        public ScrollPanel() {
            setLayout(new BoxLayout(this, BoxLayout.Y_AXIS));
            setBorder(new EmptyBorder(3,3,3,3));
        }

        @Override
        public final Dimension getPreferredScrollableViewportSize() {
            return getPreferredSize();
        }

        @Override
        public final int getScrollableUnitIncrement(Rectangle visibleRect,
                int orientation, int direction) {
            Dimension dim = panels.get(0).getComponent().getPreferredSize();
            int size;
            if (orientation == SwingConstants.HORIZONTAL) {
                size = dim.width;
            } else {
                int height = dim.height + STRUT_SIZE;
                if (direction > 0)
                    size = height - visibleRect.y % (height);
                else
                    // 1 to height, not 0 to height - 1
                    size = (visibleRect.y + visibleRect.height - 1) % (height) + 1;
            }
            return size;
        }

        @Override
        public final int getScrollableBlockIncrement(Rectangle visibleRect,
                int orientation, int direction) {
            Dimension dim = panels.get(0).getComponent().getPreferredSize();
            int size;
            if (orientation == SwingConstants.HORIZONTAL) {
                size = dim.width;
            } else {
                int height = dim.height + STRUT_SIZE;
                size = (visibleRect.height / height) * height;
            }
            return size;
        }

        @Override
        public final boolean getScrollableTracksViewportWidth() {
            return true;
        }

        @Override
        public final boolean getScrollableTracksViewportHeight() {
            if(outerPane.getViewportBorderBounds().height > getMinimumSize().height) {
                return true;
            }
            return false;
        }
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ +1 nice code, good thought-out question. One small note: are Componentable and PanelScrollable both purposefully left with the default access restriction? Otherwise, I'd recommend explicitly having them public, protected, or private. \$\endgroup\$
    – asteri
    Apr 17 '14 at 17:44
  • \$\begingroup\$ @JeffGohlke Yes they are purposefully package-private. I suppose they could be protected, but the idea is that I have a package mydomain.view which is all interfaces and this class is in package mydomain.view.swing, which is a Swing implementation of those interfaces. To disallow the presenter from ever accessing Swing methods, they cannot be public, and I don't really expect to be overriding them in a different package. protected final might make sense but I don't like final methods because it forces me to use PowerMock in testing. And private is obviously wrong. \$\endgroup\$
    – durron597
    Apr 17 '14 at 19:03
4
\$\begingroup\$

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<E> 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!

\$\endgroup\$
4
  • \$\begingroup\$ Thanks for such positive feedback! Makes me feel good on a Monday morning. A few comments from me, though (going to do it over three comments). 1) I noticed (after I made this post) that I've been making many lists/arrays of the same length and are always linked together; "A useful test is to ask yourself what would happen if you removed a piece of data or a method. What other fields and methods would become nonsense?" - Martin Fowler. I should almost certainly make a class here. \$\endgroup\$
    – durron597
    May 19 '14 at 14:15
  • \$\begingroup\$ 2) Do you have any workflow procedures for how you reorder class members? Or, even better, is there an IDE tool that does this for you? Eclipse has "sort members" but I think that's alphabetical order, not useful... \$\endgroup\$
    – durron597
    May 19 '14 at 14:16
  • \$\begingroup\$ 3) This review didn't talk about the main thing that I wanted help with: how to tease apart the logic from swing method calls so I could write automated tests. Any suggestions on that? \$\endgroup\$
    – durron597
    May 19 '14 at 14:16
  • \$\begingroup\$ @durron597 Concerning reordering. Eclipse can move selections. Just mark your method and press alt+Arrow up/down. About teasing apart logic from swing method calls... your logic is already nicely parted from what I see.. if you want to test it completely separate, you should extract it to a different class \$\endgroup\$
    – Vogel612
    May 19 '14 at 14:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.