Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am building an app for android to control VLC (mediaplayer) that runs on my computer. At the moment it's just a prototype that works, but I was wondering if I am correctly managing my HandlerThread and Socket.

My fear is that the thread keeps running too long or that I cause memory leaks. I have looked at a lot of examples and questions on Stack Overflow, but I want to be sure before I complete the app in this way.

The Java program that runs on the desktop runs only on the main thread, so I will just post my Android code.

VlcRemoteActivity.java:

package be.zweetinc.PcController;

import android.app.Activity;
import android.os.Bundle;
import android.os.HandlerThread;
import android.os.Message;
import android.view.View;
import android.widget.Button;
import android.widget.EditText;
import android.widget.ImageButton;

public class VlcRemoteActivity extends Activity {

    private ImageButton volumeUpButton;
    private ImageButton volumeDownButton;
    private EditText messageText;
    private Message message;
    private Button sendButton;
    private Button killServerButton;

    private HandlerThread connectionHandlerThread;
    private ConnectionHandler connectionHandler;

    public void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.main);
        volumeDownButton = (ImageButton) findViewById(R.id.downButton);
        volumeUpButton = (ImageButton) findViewById(R.id.upButton);
        messageText = (EditText) findViewById(R.id.message);
        sendButton = (Button) findViewById(R.id.sendButton);
        killServerButton= (Button) findViewById(R.id.killServerButton);

        connectionHandlerThread = new HandlerThread("ConnectionThread");
        connectionHandlerThread.start();

        connectionHandler = new ConnectionHandler(connectionHandlerThread.getLooper());

        message = Message.obtain(connectionHandler);
        message.what = MessageCode.CLASS_CONNECTION; // EventClass CONNECTION
        message.arg1 = MessageCode.CONNECTION_CONNECT; // EventAction CONNECT

        connectionHandler.sendMessage(message);

        volumeUpButton.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View view) {
                message = Message.obtain(connectionHandler, MessageCode.CLASS_VLC, MessageCode.VLC_VOLUME_UP, 0);
                connectionHandler.sendMessage(message);
            }
        });

        volumeDownButton.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View view) {
                message = Message.obtain(connectionHandler, MessageCode.CLASS_VLC, MessageCode.VLC_VOLUME_DOWN, 0);
                connectionHandler.sendMessage(message);
            }
        });

        sendButton.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View view) {
                String textToSend = messageText.getText().toString();

                Bundle data = new Bundle();
                data.putString(ConnectionHandler.MESSAGE_DATA, messageText.getText().toString());
//                TODO: change arg1 and arg2 parameter when feature is not limited to logging only
                message = Message.obtain(connectionHandler, MessageCode.CLASS_VLC, MessageCode.VLC_VOLUME_UP, 0, data);
                connectionHandler.sendMessage(message);
            }
        });

        killServerButton.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View view) {
                message = Message.obtain(connectionHandler, MessageCode.CLASS_SERVICE, 0, 0);
                connectionHandler.sendMessage(message);
            }
        });
    }

    @Override
    protected void onPause() {
        super.onPause();
        message = Message.obtain(connectionHandler, MessageCode.CLASS_CONNECTION, MessageCode.CONNECTION_DISCONNECT, 0);
        connectionHandler.sendMessage(message);
//        connectionHandlerThread.quit();
    }

}

ConnectionHandler.java:

package be.zweetinc.PcController;

import android.os.Handler;
import android.os.Looper;
import android.os.Message;
import android.util.Log;
import org.json.JSONException;
import org.json.JSONObject;

import java.io.IOException;
import java.io.PrintWriter;
import java.net.Socket;

public class ConnectionHandler extends Handler {
    public static final String MESSAGE_DATA = "be.zweetinc.PcController.Message_Data";

    private Socket client;
    private PrintWriter printwriter;
    private Message message;

    public ConnectionHandler(Looper looper){
        super(looper);
    }

    @Override
    public void handleMessage(Message msg) {
        super.handleMessage(msg);    //To change body of overridden methods use File | Settings | File Templates.
        message = msg;
        if(message.what != MessageCode.CLASS_CONNECTION){
            sendMessageToServer();
        } else {
            handleConnection();
        }
    }
    private void handleConnection(){
        if(message.arg1 == MessageCode.CONNECTION_CONNECT) {
            makeConnection();
        } else {
            closeConnection();
        }
    }

    private void sendMessageToServer(){
        JSONObject jsonObject = new JSONObject();
        try {
            jsonObject.put("eventClass", message.what);
            jsonObject.put("eventAction", message.arg1);
            jsonObject.put("eventActionType", message.arg2);
            jsonObject.put("eventText", message.getData().getString(MESSAGE_DATA));
        } catch (JSONException e) {
            e.printStackTrace();  //To change body of catch statement use File | Settings | File Templates.
        }



        printwriter.println(jsonObject.toString());

        Log.d("ConnectionInfo", "printwriter error: " + printwriter.checkError());

//        printwriter.write(jsonObject.toString()+"\n");
    }

    protected void quit(){
        getLooper().quit();
    }

    private void makeConnection(){
        try {
            client = new Socket("192.168.1.195", 4444);
            Log.d("ConnectionInfo", client.toString());
            printwriter = new PrintWriter(client.getOutputStream(), true);
        } catch (IOException e) {
            e.printStackTrace();  //To change body of catch statement use File | Settings | File Templates.
        }
    }

    private void closeConnection(){
        printwriter.close();
        try {
            client.close();
        } catch (IOException e) {
            e.printStackTrace();  //To change body of catch statement use File | Settings | File Templates.
        }
        quit();
    }
}
share|improve this question
add comment

1 Answer

up vote 5 down vote accepted

From what I can see, it seems like you are doing it correctly.

On some exceptions, you might want to alert your user with a message about what went wrong (for example, "Connection failed"). Few users can read stack traces while using an app.

I only have a few comments regarding your code style:

You're inconsistent in putting spaces around {-characters, and also with if-statements. There should always be a space after if and before a {
As in, private void makeConnection(){ <-- insert space here.

share|improve this answer
    
Thanks for the answer, I will indeed be adding user alerts, the code I posted was a simple proof of concept to test what the best way for the use of a socket would be. As for my coding style, fair points, I will keep them in mind for my future coding/refactoring. –  Tweet Nov 22 '13 at 10:22
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.