How to improve this design?

Question

I have some design doubts regarding my controllers and I would like some advice... Everything works. However, I'd like to improve it.

Basically I have one controller per screen(Swing). I have created the following structure:

// This interface has methods that all controllers must have
public interface Controller {

    public void cancel();   
    public void openPanel() throws DomainComponentException;
}

// This interface represents what most of the screens must have 
public interface DefaultController<T> extends Controller {

    public void newButton();    
    public void save();
    public void edit();
    public void search();
    public void delete();
    public T getSelectedRow() throws BusinessException;
    public void validate(T t) throws BusinessException, DomainComponentException;
    public AbstractTableModel searchAll();  
}
// For each controller I have an interface, even not having any additional method
// This interface is also used to easily wire the Controller.
public interface ClientController extends DefaultController<Client> {

}   

//This is my concrete class
@Component
public class ClientControllerBean implements ClientController {

    @Inject
    private ClientService clientService;
    private ClientPanel clientPanel;
    private SearchPanel searchPanel;

    public void newButton() {
        openPainel();
        clientPanel.updateFields(Action.NEW_BUTTON);
    }

    public void save() {
        try {
            Client client = clientPanel.getInterfaceData();
            validar(client);
            client = clientService.save(client);
            JOptionPane.showMessageDialog(null, "Sucessfully saved!");              
            ComponentUtil.enableButtons(Action.SAVE_BUTTON);
            search();
        } catch (Exception e) {
            ExceptionUtil.handleException(e, clientPanel, true);
        }
    }

    public void edit() {
        try {
            Client client = getSelectedRow();
            openPanel();
            clientPanel.setInterfaceData(client);
            ComponentUtil.enableButtons(Action.EDIT_BUTTON);
        } catch (Exception e) {
            ExceptionUtil.handleException(e, clientPanel);
        }
    }

    public Client getSelectedRow() throws BusinessException {
        ClientTableModel clientTableModel = Util.getTableModel(ClientTableModel.class, searchPanel.getTable());
        int selectedRow = searchPanel.getTable().getSelectedRow();
        Client client = clientTableModel.getSelectedClient(selectedRow);
        return client;
    }

    public void search() {
        DesktopMain.getTitledPanel().setTitle("Search Client");
        searchPanel = new searchPanel(this);
        searchPanel.loadData();
        ComponentUtil.displayComponent(searchPanel, Option.DEFAULT_SCREEN);
        ComponentUtil.enableButtons(Action.SEARCH_BUTTON);
    }

    public void delete()  {
        try {
            Client client = getSelectedRow();
            Integer reply = JOptionPane.showConfirmDialog(null, Constants.DELETE_SELECTED, Constants.DELETE ,JOptionPane.YES_NO_OPTION);
            if (reply.equals(0)) {
                clientService.delete(client);
                JOptionPane.showMessageDialog(null, Constants.DELETED);
                search();
            }
        } catch (Exception e) {
            ExceptionUtil.handleException(e, clientPanel, true);
        }
    }

    public void cancel() {
        if (clientPanel != null && clientPanel.isVisible()) {
            Integer reply = JOptionPane.showConfirmDialog(null, Constants.DATA_WILL_NOT_BE_SAVED, Constants.CANCEL,JOptionPane.YES_NO_OPTION);
            if (reply.equals(0)) {
                ComponentUtil.backInitialScreen();
            }
        } else {
            ComponentUtil.backInitialScreen();
        }
    }

    public AbstractTableModel searchAll() {
        List<Client> clients = null;
        try {
            clients = clientService.searchAll();
        } catch (DomainComponentException e) {
            clients = new ArrayList<Client>();
            ExceptionUtil.handleException(e, null, true);
        }   
        AbstractTableModel tableModel = new ClientTableModel(clients);
        return tableModel;
    }

    public void openPanel() {
        DesktopMain.getTitledPanel().setTitle("Client");
        clientPanel = new clientPanel(this);
        ComponentUtil.displayComponent(clientPanel, Option.DEFAULT_SCREEN, true);
    }

    public void validate(Client client) throws BusinessException {
        Map<String, String> invalidFields = new HashMap<String, String>();

        ValidationUtil.validate(client, invalidFields);     

        ComponentUtil.updateFields(clientPanel, invalidFields);
    }   
}

I have some methods that will be pratically the same for other screens. Ex: newButton() This method will be the same for most screens. cancel() The only thing different here will be the "panel" instance

Having it in mind, I'd like to place this logic in a separeted place. I thought about placing it in a Helper or use an abstract base controller. Nevertheless, I'd like to avoid inheritance, maybe use composition, or a decorator?

So, I'do like to hear some ideas.

Thanks, Diego

Answer 1

If I understood correctly you have several data objects classes like Client. And you want to generalize UI user interaction by Controller hierarchy. Which is UI screens with typical behavior:

• User data interaction:

  1. new
  2. save
  3. edit
  4. search
  5. delete
  6. validate

• UI general operations:

  1. open
  2. cancel
  3. get selected element

From what I see:

1. I wouldn't generalize around data object class (Client) in Controller, but would generalize around data provider class (ClientService). I don't see any client specific functionality but I see a lot of interaction with ClientService
2. I wouldn't expose that you're using table here. I would create next level of hierarchy where you could implement representation type - table, tree or something else

But I would do this only if you have really several different representation and object data types.

General comments:

1. I would think more about class names - Controller and DefaultController don't give too much explanation. Methods names - SearchAll is probably nothing about searching. ExceptionUtil would probably better named as ExceptionHandler
2. I didn't understand why inside save and delete we call search
3. I hope ExceptionUtil some warning to user to notify about exceptional state. As well as I don't really like using general Exception catch block as way of catching several exception types in one place