Am I using this Handler and Executor correctly (Android)?

Question

I have an Executor that run a while true loop that just pings a server with data. Now I need to updated the UI with this data, so I am using a handler.

In my main activity I have:

private void createGeneralHandler() {
    generalHandler = new Handler(Looper.getMainLooper()){
        @Override


    public void handleMessage(Message msg){

                switch (msg.what){
                    case SERVER_RESPONSE:

                        serverTextView.append("\n"+msg.obj);
                        break;

                    default:
                        super.handleMessage(msg);
                }
            }
        };
    }

This creates a field in the main activity called generalHandler.

Now the NetworkTask runnable needs to know about this handler in order to send the messages to right ?

So I have this in my main activity:

networkExecutor = Executors.newSingleThreadExecutor();
networkTask = new NetworkTask(serverIPAddress, serverPort);
networkTask.setRequest("I WANT DATA");
networkTask.setHandler(generalHandler); //IS THIS WRONG ???
networkExecutor.execute(networkTask);  

networkTask is just a Runnable in a separate file defined as such:

public class NetworkTask implements Runnable {

    private int port;
    private String ipAddress;
    private int pollInterval;
    private Socket socket = null;
    private PrintWriter out = null;
    private BufferedReader br = null;
    private String request = null;
    private String response = null;
    private static final int SERVER_RESPONSE = 1;
    private Handler handler = null;

    public NetworkTask(String ipAddress, int port){
        this.port = port;
        this.ipAddress = ipAddress;
        this.pollInterval = 50;
    }

    @Override
    public void run() {

        try {
            socket = new Socket(ipAddress, port);
            out = new PrintWriter(socket.getOutputStream(), true);
            br = new BufferedReader(new InputStreamReader(socket.getInputStream()));
        } catch (IOException e) {
            e.printStackTrace();
        }

        try {
            while (!Thread.currentThread().isInterrupted()) {

                Thread.sleep(pollInterval);

                out.println(request);
                try {
                    response = br.readLine();
                } catch (IOException e) {
                    e.printStackTrace();
                }
                if(!response.equals("doNothing")){
                    Message message = new Message();
                    message.what = SERVER_RESPONSE;
                    message.obj = response;
                    handler.sendMessage(message);
                }else if(response.equals("Nothing Connected to Server!")){
                    Message message = new Message();
                    message.what = SERVER_RESPONSE;
                    message.obj = response;
                    handler.sendMessage(message);
                }
            }
        } catch (InterruptedException threadE) {
            try {
                br.close();
                out.close();
                socket.close();
            } catch (IOException socketE) {
                socketE.printStackTrace();
            }
            Log.i("NETWORK_TASK_THREAD", "NETWORK TASK closed gracefully!");
        }

    }

    public void setRequest(String request) {
        this.request = request;
    }

    public void setHandler(Handler handler) {
        this.handler = handler;
    }
}

Answer 1

Answer to your question will be a bit arbitrary. Since the implementation is just a design choice, there won't be right or wrong but would be good or not so good.

Following are points which I would like to highlight:

• If you are just concerned networkTask.setHandler(generalHandler); then its fine as long as you are not invoking any network call or heavy processing on Main thread.
• You also need to ensure that your Runnable is interrupted appropriately in apps onStop() or onDestroy() to avoid potential memory leak.
• You are continuously trying to ping the server which is not a great idea since it will consume resources and drain battery. You can perform these tasks periodically or use FCM push.
• For closing the InputStreams or OutputStreams its considered good practice to clean up in finally {..} block which guarantees clean up in case some other Exceptions occurs during execution.

One alternative would be to implement ViewModel and MutableLiveData and perform the operation. This will no bloat your Activity with un-necessary processing.