Swing UI for a test Websocket client

I am new to Java's Swing and built a test client that connects to a server. I was wondering if my use of a Controller class as a link between the classes ClientWindow and WebsocketService made sense and would be thankful for comments. (I can take it, so the more my code is criticized the happier I am.)

For the sake of brevity, I have omitted most methods and fields.

ClientWindow

public class ClientWindow implements ActionListener {
    private static final String USER_ID = "19";
    private final IController controller;

    @Override
    public void actionPerformed(ActionEvent e) {
        controller.connectAs(USER_ID);
    }

    public void addMessage(String line) {
        responseArea.setText( responseArea.getText() + line );
    }
}

DefaultController (implements IController)

public class DefaultController implements IController {

    protected WebsocketService wsService;
    protected ClientWindow display;

    public DefaultController() {
        wsService = new WebsocketService(this);
    }

    public void setDisplay(IClientDisplay display) {
        this.display = display;
    }

    // ------------------
    // IController methods
    @Override
    public void updateMessage(String message) {
        if (display != null) {
            display.addMessage(message);
        }
    }

    @Override
    public void connectAs(String userId) {
        wsService.connectToServer(userId);
    }
}

WebsocketService

```
@ClientEndpoint
public class WebsocketService {
private final IController controller;
private String userId;

public WebsocketService(IController controller) {
this.controller = controller;
}

/**
* On open: register the session and signal
* the user ID and to the Node.js server.
* @param session The WebSocket session
*/
@OnOpen
public void onOpen(Session session) {
this.session = session;
controller.updateMessage("Connected to server");
send(

-
I'm not too familiar with GUIs nor MVC, but I guess your model contains at least a string (which is displayed in the responseArea). You might put that into a separate model class.

Another thing is a websocket service which I try to hide behind the model and separate it from the controller class. Wikipedia says the following about MVC's model:


A model notifies its associated views and controllers when there has been a change in its state.

So, the websocket service could notify the model, then the model would notify the view about the change.

-
I'd rename IController to Controller.

Clean Code by Robert C. Martin, Chapter 2: Meaningful Names, p24:


Interfaces and Implementations


These are sometimes a special case for encodings. For example, say you are building an
ABSTRACT FACTORY for the creation of shapes. This factory will be an interface and will
be implemented by a concrete class. What should you name them? IShapeFactory and
ShapeFactory? I prefer to leave interfaces unadorned. The preceding I, so common in
today’s legacy wads, is a distraction at best and too much information at worst. I don’t
want my users knowing that I’m handing them an interface. I just want them to know that
it’s a ShapeFactory. So if I must encode either the interface or the implementation, I choose
the implementation. Calling it ShapeFactoryImp, or even the hideous CShapeFactory, is
preferable to encoding the interface.

-
I think WebsocketService should not know anything about Controller.connectAs. Currently it violates the interface-segregation principle. You could create a WebsocketCallback interface which your controller implementation class could implement:

public interface WebsocketCallback {
    void updateMessage(String message);
}

public class DefaultController implements Controller, WebsocketCallback {
    ...
}

I guess implementing it by the model class could be even better (see #1).

-
I would rename the parameter here to message to express its purpose and make the code easier to understand:

public void updateMessage(String string) {

-
I would throw here an exception instead of hiding the error:

@Override
public void updateMessage(String message) {
    if (display != null) {
        display.addMessage(message);
    }
}

Crash early. It is rather a bug in the code when it is called without setting the display before which you shouldn't hide. It would help debugging: having a stacktrace usually better than just an one-line bugreport: "the button does not do anything" or something similar. See also: The Pragmatic Programmer: From Journeyman to Master by Andrew Hunt and David Thomas: Dead Programs Tell No Lies.

-
If responseArea if a JTextArea instance, you could use append instead of setText(getText() ...) here:

responseArea.setText( responseArea.getText() + line );

StackExchange Code Review Q#57363, answer score: 3