10
\$\begingroup\$

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( "c:" + userId);
    }

    /**
     * On message: handle incoming message by
     * invoking methods of the controller. 
     * @param message
     */
    @OnMessage
    public void onMessage(String message) {
        controller.updateMessage(message);
    }

    /**
     * Open a WebSocket to connect to the Node.js server.
     */
    public void connectToServer(String userId) {
        this.userId = userId;
        WebSocketContainer container = ContainerProvider.getWebSocketContainer();
        try {
            container.connectToServer(this, new URI(Commons.SERVER_ADDR));
        }
        // catch blocks
    }
}

Finally, Client

public class Client {   
    public static void main(String[] args) {
        DefaultController controller = new DefaultController();
        controller.setDisplay(new ClientWindow(controller));
    }
}

Client definitely looks bad, especially that second line. I might as well skip the DefaultController and just have a field for ClientWindow in WebsocketService and vice versa but that's obviously a big no-no. I have looked into MVC but I don't really have much to model (or am I wrong?).

After all, I need to be able to give commands to my Service but also to display output from it at any given time (onMessage) and so this was the most elegant solution I could think of.

\$\endgroup\$
  • \$\begingroup\$ I haven't looked at this in too much detail, but that random hard coded "19" looks pretty smelly. I can see later it's a userId, but whose? \$\endgroup\$ – raptortech97 Jul 18 '14 at 10:49
  • \$\begingroup\$ @raptortech97 Aha, definitely :) It's just a stupid example so I don't have to post the code to get the input from the user, which is not relevant to my question. \$\endgroup\$ – ljacqu Jul 18 '14 at 11:13
  • \$\begingroup\$ I wanted to review this but I genuinely can't think of any improvements to what I see here. Good job :) \$\endgroup\$ – Eddie Curtis Jul 19 '14 at 10:46
3
\$\begingroup\$
  1. 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.

  2. 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.

  3. 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).

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

    public void updateMessage(String string) {
    
  5. 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.

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

    responseArea.setText( responseArea.getText() + line );
    
\$\endgroup\$
  • 1
    \$\begingroup\$ Beautiful answer, many thanks for your time! \$\endgroup\$ – ljacqu Jul 23 '14 at 11: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.