1
\$\begingroup\$

I have to create a client-server JavaFX application using MVC which does many operations consisting mainly in messages exchange. I'd like to know if this way to handle client acceptance and client requests is good or not.

public class ServerOperations {

// other code

public void serverConnection() {
    Runnable runnable = new AcceptingClients();
    Thread thread = new Thread(runnable);
    thread.setDaemon(true);
    thread.start();
}

public class AcceptingClients extends Thread {

    static final int PORT = 9090;
    private Socket socket = null;

    ServerSocket serverSocket = null;
    @Override
    public void run() {
        try {
            serverSocket = new ServerSocket(PORT);
        } catch (IOException e) {
            System.out.println("I/O error: " + e);
        }
        while (true) {

            try {
                socket = serverSocket.accept();

                // other code

                Runnable runnable = new ClientRequests(socket, inputStream, outputStream);
                Thread thread = new Thread(runnable);
                thread.setDaemon(true);
                thread.start();

            } catch (IOException e) {
                System.out.println("I/O error: " + e);
            } catch (ClassNotFoundException ex) {
                Logger.getLogger(ServerRequests.class.getName()).log(Level.SEVERE, null, ex);
            }
        }
    }

}

public class ClientRequest extends Thread {
  // other code
  public ClientRequest(Socket socket) throws ClassNotFoundException {
         // other code
  }

@Override
public void run() {
       // handling client requests
 }
// other code
}

I want to focus on few parts of this class ServerOperations:

1) Is good to put all those thread in one class for accepting clients and handling requests or is it bad coding (if not, what a good alternative would be)? Using this way may give me problems if I have a lot of clients connected?

2) If i don't extend the class AcceptingClients with Thread I have problem with JavaFX UI thread, I can't add text in TextArea for example etc. I can't understand from where this interference come from, not even using Platform.runLater() makes me append text or interact with my controller's components.

3) To let everything run I have a method in my model where this method serverConnection() is called. When the Server class starts, the first thing it does is to open the server UI and run this method called from the model. Good or not?! Note: Server class is different from ServerOperations. ServerOperations is a separate classe which just handle client acceptance and requests.

\$\endgroup\$
1
\$\begingroup\$

Note: It is easier to provide a more complete answer when faced with a complete problem. There are several //other code areas that you may be omitting for the sake of brevity, but these could very well be providing more context to the review, and their presence could likely improve the quality of answers received.

I would recommend posting complete code as is, using a more descriptive place holder of the logic there than //other code and/or perhaps an explanation of why the missing code is considered irrelevant to the reviewer

/rant


Request for clarification I noticed that in your AcceptingClients Thread you make a call to new ClientRequests(socket, inputStream, outputStream). However, you also define the similarly named ClientRequest with constructor public ClientRequest(Socket socket). Are these meant to be the same class, just with the latter being un-implemented? or are these differing classes, in which case I would recommend better names


On to your questions 3:

1) Is it good to put all those thread in one class

Having a class ServerOperations containing each of your Operation threads IMO is actually good design (so long as each operation is reasonably simple).

When it comes to denser and thus more complex logic, I would instead opt for grouping each Operation class into a subdirectory /operations and using ServerOperations like a repository; a layer for accessing your collection of operations, something like ServerOperations.getOperationThread("acceptClients").

The benefit here is readability and maintainability. IMHO It is easier to understand and thus maintain the individual processes if they are in separate class files, but this would only be recommended for a collection of non-trivial Operations. If they are super simple, feel free to group them up like this!

Using this way may give me problems if I have a lot of clients connected?

Short answer, yes. Based on the present code, I would highly recommend using non-daemon threads for these purposes. This question on SO has a TON of discourse on daemon threads but IMO the most relevant information therein is :

Daemon threads are service providers for user threads running in the same process.

This means any Thread with setDaemon(true) should be considered a service, something that is running in the background and waiting to service a request from another, non-daemon thread. This leads the most annoying impact:

Daemon threads can shut down any time in between their flow, Non-Daemon i.e. user thread executes completely.

Normally, if your program were ending, it would hang on exiting the JVM until all user threads are complete. As these threads are daemons, the JVM will drop these Threads and exit immediately. This means that in the edge case of a request still processing on shutdown, you will lose requests processing in these daemons.

Without information on how this daemon is used in these //other code sections, I can only conclude that you might not need to use daemonThreads


2) If i don't extend the class AcceptingClients with Thread I have problem with JavaFX UI thread

I am unfamiliar with JavaFX so I won't offer advice I am unqualified to give


3) To let everything run I have a method in my model where this method serverConnection() is called. [..] Good or not?!

If I am understanding this correctly, you are asking if it is good to call this method from the Model at creation (IMO that isn't a bad choice in the general sense). This question is incredibly difficult to answer without having the Model code available for context.

IMHO, this last is a question belonging in a review of the Model, or of the project as a whole, as without seeing the complete context of HOW this method is used, any feedback would basically be personal opinions about the proper use of MVC models

Hope some if not all of this helps!

\$\endgroup\$
  • \$\begingroup\$ Thank you so much for answering, you've been very helpful! I'm sorry for all those //other code but this is part of a school project and I can't post the whole code since it's easy to reach this post and copy! The public ClientRequest(Socket socket) it's a mistake, I think I deleted something while copy/paste. They are the same class anyway! \$\endgroup\$ – NoProg Mar 30 at 15:56

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.