Communication with GARMIN through WEB

Question

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)
 {
 System.out.println("SocketException initConnection() ");
 e.printStackTrace();
 }
 catch(IOException e)
 {
 System.out.println("Exception initConnection()");
 e.printStackTrace();
 }
 finally
 {
 if(shutDownServer)
 {
 System.out.println("Breaking while looop for client listening");
 loopStatus = false;
 }
 } 
 }
 }
 });
 clientSocketListener.setPriority(3);
 clientSocketListener.start();
 }
 @Override
 public void run() 
 {
 this.initConnection();
 System.out.println("After calling initConncetion()");
 byte currentByte = 0;
 byte previousByte = 0;
 boolean firstRun = true;
 LinkedList<Byte> inputDataArray = new LinkedList<Byte>();
 while(stopLoop == false)
 {
 try 
 { 
 if(currentSocket != null)
 {
 in = new DataInputStream(currentSocket.getInputStream());
 System.out.println("After checking if currentSocket is null");
 int nextByte;
 while((nextByte = this.in.read()) != -1)
 {
 if(firstRun)
 {
 firstRun = false;
 previousByte = (byte) nextByte; //in.readByte();
 if(previousByte == (byte) 0xa5) inputDataArray.add(previousByte);
 }
 else
 {
 currentByte = (byte) nextByte; //in.readByte();
 if(previousByte == 21 && currentByte == 22)
 {
 inputDataArray.clear();
 System.out.println("Na4alo...");
 }
 else if(previousByte == 23 && currentByte == 24)
 {
 //Do something here...
 if(type.equals(GarminMessage.MESSAGE_TYPE_GARMIN_TEXT))
 {
 //Do what is supposed to do...
 }
 else if(type.equals(GarminMessage.MESSAGE_TYPE_GARMIN_ACKNOWLEDGEMENT))
 {
 //Do what is supposed to do... 
 }
 else if(type.equals(GarminMessage.MESSAGE_TYPE_SYNCHRONIZE_POINT_ACCNOWELDGEMENT))
 {
 //Do what is supposed to do... 
 }
 else if(type.equals(GarminMessage.MESSAGE_TYPE_ETA_ACCNOWELDGEMENT))
 {
 //Do what is supposed to do... 
 }
 inputDataArray.clear();
 firstRun = true;
 System.out.println("Krai!");
 }
 else
 {
 inputDataArray.add(currentByte);
 previousByte = currentByte;
 if(inputDataArray.size() == 5)
 {
 boolean flag = true;
 for(int i=0; i<inputDataArray.size(); i++)
 {
 byte a = inputDataArray.get(i); //System.out.println("data: " + a);
 if(a != (byte) 0xa5)
 {
 flag = false;
 break;
 }
 }
 if(flag == true)
 {
 //System.out.println("Handshake message received");
 inputDataArray.clear();
 firstRun = true;
 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!");
 if(MessageService.this.messageSender.out != null)
 {
 try {MessageService.this.messageSender.out.close();} 
 catch (IOException e) {} 
 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();
 }
 catch(Exception ex)
 {
 System.out.println("Exception: Connection has been lost and the handshake message was not sent!");
 if(MessageService.this.messageSender.out != null)
 {
 try {MessageService.this.messageSender.out.close();} 
 catch (IOException e) {} 
 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();
 }
 }
 }
 }
 }
 }
 } 
 } 
 catch (IOException ex) 
 {
 ex.printStackTrace();
 System.out.println("IOException inside while loop of run method of thread message receiver");
 inputDataArray.clear();
 this.closeInputStream();
 this.closeSocket();
 }
 catch(Exception e)
 {
 e.printStackTrace();
 System.out.println("Exception");
 inputDataArray.clear();
 this.closeInputStream();
 this.closeSocket();
 }
 finally
 {
 try {Thread.sleep(200);} 
 catch (InterruptedException ex)
 {
 Thread.currentThread().interrupt();
 break;
 }
 }
 } //end of while(true)
 System.out.println("Outside while loop");
 //shutDownServer = true;
 }
}

Answer 1

1. 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();
    }
   }
   

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

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

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

5. 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;
    }
    ...
    }
    ...
   }
   

6. 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) {
    ...
   }
   

7. 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();
    }
   }
   

8. 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 (IOException e) {
    }
   }
   

   Usage:

    private void closeAndReconnect() {
    try {
    closeQuietly(MessageService.this.messageSender.out);
    closeQuietly(MessageService.this.messageSender.mSenderSocket);
    } finally {
    MessageService.this.messageSender.out = null;
    MessageService.this.messageSender.mSenderSocket = null;
    }
    MessageService.this.messageSender.initConnection();
   }
   

   Anyway, it's usually not a good idea to swallow exceptions. You should at least log them, it could help with "doesn't work" bugs where there is no other clue what has happened.

9. Numbers like, 0xa5, 21, 24 are magic numbers. Using a named constants would help readers/maintainers because named constants could express the purpose of value, they would reveal the intent, what the original author wanted to achieve with the number.

10. boolean shutDownServer = false;
    

    shutdown is one word, I'd write it with lowercase d.

11. private ServerSocket mReceiverSocket = null; 
    

    The m prefix is rather unnecessary.

12. LinkedList<Byte> inputDataArray = new LinkedList<Byte>();
    

    LinkedList<...> reference types should be simply List<...>. See: Effective Java, 2nd edition, Item 52: Refer to objects by their interfaces

13. boolean flag = true;
    

    I more descriptive name would help readers, since every boolean is a flag it doesn't say too much.

• Joro Seksa: 10x. It's just amazing how well you explain stuff. I have to post questions more often here