Communication with GARMIN through WEB

I am developing a WEB based GPS system and one of its functionalities is to be able to send messages and routes to GARMIN devices.

I have never been using sockets before, and for some (obvious) reason, I don't feel ready to submit my code before being reviewed by more experienced users.

If some of you have the time to check it, here it is:

```
//This is an inner class - that is why it is private
private class MessageReceiver implements Runnable
{
private Thread clientSocketListener = null;

private Socket clientSocket = null;
private Socket currentSocket = null;

private ServerSocket mReceiverSocket = null;

private DataInputStream in = null;

boolean shutDownServer = false;
boolean stopLoop = false;

public MessageReceiver()
{
try
{
if(mReceiverSocket == null) { mReceiverSocket = new ServerSocket(12346); }
System.out.println("mReceiverSocket initialized");
}
catch (IOException ex) {ex.printStackTrace();}
}

/**
* Listens for clients...
*/
private void initConnection()
{
clientSocketListener = new Thread(new Runnable()
{

private boolean loopStatus = true;

@Override
public void run()
{
while(loopStatus)
{
try {Thread.sleep(200);}
catch (InterruptedException ex) {ex.printStackTrace();}

try
{
clientSocket = mReceiverSocket.accept();

if(currentSocket != null) { currentSocket.close(); }

currentSocket = clientSocket;
System.out.println("new clientSocket accepted");
}
catch(SocketException e)

-

private ServerSocket mReceiverSocket = null;
...
public MessageReceiver()
{
    try 
    {
        if(mReceiverSocket == null)  { mReceiverSocket = new ServerSocket(12346); }
        System.out.println("mReceiverSocket initialized");
    } 
    catch (IOException ex) {ex.printStackTrace();}
}

The if condition inside the constructor is never false. It could be simply the following:

private ServerSocket mReceiverSocket;
...
public MessageReceiver() {
    try {
        mReceiverSocket = new ServerSocket(12346);
        System.out.println("mReceiverSocket initialized");
    } catch (IOException ex) {
        ex.printStackTrace();
    }
}

-
Allowing to create an object with an invalid state (when receiverSocket is null) does not look a good idea. You'll get a NullPointerException later, usually when it runs on another thread. It's harder to debug since the stacktrace will not contain any clue where was the MessageReceiver created. So, instead of printStackTrace throw an exception immediately. (The Pragmatic Programmer: From Journeyman to Master by Andrew Hunt and David Thomas: Dead Programs Tell No Lies.)

-

boolean shutDownServer = false;
boolean stopLoop = false;

I guess it's not thread-safe. Access to the fields above should be synchronized, otherwise not all thread will see that their values are changed.


[...] synchronization has no effect unless both read and write operations are synchronized.

Source: Effective Java, 2nd Edition, Item 66: Synchronize access to shared mutable data


Unless synchronization is used every time a variable is accessed,
it is possible to see a stale value for that variable. Worse,
staleness is not all-or-nothing: a thread can see an up-to-date
value of one variable but a stale value of another variable that
was written first.

Source: Java Concurrency in Practice, 3.1.1. Stale Data

Consider using AtomicBooleans.

-
Fourteen level of indentation is hard to read (you need to scroll horizontally), you should extract out some methods and classes to fulfil the single responsibility principle. Having small classes and methods means that you don't have to understand the whole program when maintaining it later to modify just a small part of it which is easier and requires less work.


The first rule of functions is that they should be small.
The second rule of functions is that
they should be smaller than that.

Source: Clean Code by Robert C. Martin, Chapter 3: Functions

-
In the run method a simple guard clause could save you an indentation level.

while(stopLoop == false)
{
    try 
    {                    
        if(currentSocket != null)
        {
            ...
        }
    }
    ...
}

would become

while (stopLoop == false) {
    try {
        if (currentSocket == null) {
            continue;
        }
        ...
    }
    ...
}

-
The following try-catch (with every catch blocks) could have a separate method too:

byte handShakeMessage[] = new byte[] { (byte) 0xa6, (byte) 0xa6, (byte) 0xa6, (byte) 0xa6, (byte) 0xa6 };

try {
    MessageService.this.messageSender.out.write(handShakeMessage);
    MessageService.this.messageSender.out.flush();
} catch (IOException ex) {
    ...
}

-
The content of the two catch blocks above is exactly the same, you can move that logic to a separate method too:

private void closeAndReconnect() {
    if (MessageService.this.messageSender.out != null) {
        try {
            MessageService.this.messageSender.out.close();
        } finally {
            MessageService.this.messageSender.out = null;
        }
    }

    if (MessageService.this.messageSender.mSenderSocket != null) {
        try {
            MessageService.this.messageSender.mSenderSocket.close();
        } catch (IOException e) {
        } finally {
            MessageService.this.messageSender.mSenderSocket = null;
        }
    }

    MessageService.this.messageSender.initConnection();
}

Then it gets being readable, although a good name is still missing:

private void extracted() {
    byte handShakeMessage[] = new byte[] { (byte) 0xa6, (byte) 0xa6, (byte) 0xa6, (byte) 0xa6, (byte) 0xa6 };

    try {
        MessageService.this.messageSender.out.write(handShakeMessage);
        MessageService.this.messageSender.out.flush();
    } catch (IOException ex) {
        System.out.println("IOException: Connection has been lost and the handshake message was not sent!");
        closeAndReconnect();
    } catch (Exception ex) {
        System.out.println("Exception: Connection has been lost and the handshake message was not sent!");
        closeAndReconnect();
    }
}

-
If both messageSender.out and messageSender.mSenderSocket implements Closeable you could create a closeQuietly method for them:

```
private void closeQuietly(final Closeable closeable) {
if (closeable == null) {
return;
}
try {
closeable.close();
} catch

StackExchange Code Review Q#24510, answer score: 2