6
\$\begingroup\$

I am putting together a fairly simple server that listens for a connection then creates this thread - textbook Java code - then accepts data on that connection.

I am following a protocol that the manufacturer has laid out for start of message (0xfd) and EOM (0xfe) as below. I then simply populate a byte array with using a bytecounter.

I think this is the simplest doing so, and it seems to work fine. Is there any possible problem with taking this approach? I want to be sure using a bytecounter with an array is acceptable. I can't see anyway that bytecounter could get out of sync or anything like that. I like to keeps things simple. Is there anything wrong with this code with regard to stuffing the inbyte[] array?

public void run() {

try {


    boolean socketalive = true;

    MSG_ID = 0;

    disIn = new DataInputStream( in  );
    disOut = new DataOutputStream ( out );

    String msg = null;

    StringBuilder hexforlog;

    byte[] c ;


    int i = 0;
    int bytecounter = 0;

    byte[] inbyte ;

    while( true ){



        i = 0;
        bytecounter = 0;

        inbyte = new byte[1024];

        byte b ;





        try{

            /* read from network until end of message byte is received */
            while( ( b = disIn.readByte() ) !=  (byte)0xfe ){

                /* checking for start of message */
                if( b == (byte)0xfd  ){


                    inbyte = new byte[1024];
                    inbyte[0] = b;
                    bytecounter = 1;


                }
                else {

                     inbyte[bytecounter] = b;
                     bytecounter++;

                }


            }

        }catch ( java.io.EOFException ioef ){
            System.out.println("EOF recieved bytes"  );
            break;
        }




        /*test for link verification */

        if( bytecounter == 15 ){


             byte SOURCEbyte = inbyte[2];
             byte DESTbyte = inbyte[3];

             /* swap bytes per protocol */
             inbyte[2] = (byte)DESTbyte;
             inbyte[3] = (byte)SOURCEbyte;


            disOut.write( java.util.Arrays.copyOfRange(inbyte,0,16) );
            disOut.flush();

        }





        /* process the data as per manufacturer spec */ 
        else if( bytecounter> 16  ){



            arrayindex = 12;

            int SOM = inbyte[0];
            int CLASS = inbyte[1];
            int SOURCE = inbyte[2];
            int DEST = inbyte[3];
            int MSG_PRG_NUMBER = inbyte[4];
            int SPARE1 = inbyte[5];
            int SPARE2 = inbyte[6];
            int SPARE3 = inbyte[7];

            LENGTH = twoBytesToInt( new byte[] { inbyte[8], inbyte[9] } );



            NUM_MSG = twoBytesToInt( new byte[] {  inbyte[10], inbyte[11]  } );

           System.out.println("LENGTH: " + LENGTH  + " NUM_MSG: " + NUM_MSG );
           System.out.println("Setting NUM_MSG to 1" );

            NUM_MSG = 1;




            for ( int j=0; j<NUM_MSG; j++  ) {


                MSG_ID = twoBytesToInt( new byte[] { inbyte[arrayindex],    
                     inbyte[arrayindex+1] } );

                System.out.println("MSG_ID: " + MSG_ID );

                MSG_LENGTH =twoBytesToInt( new byte[] { inbyte[arrayindex+2],  
                    inbyte[arrayindex+3] } );

                System.out.println("MSG_LENGTH: " + MSG_LENGTH  );

                arrayindex = arrayindex+4; //first go around 15



                currentarray = java.util.Arrays.copyOfRange( 
                   inbyte, arrayindex, arrayindex+MSG_LENGTH+1);

                arrayindex = arrayindex+MSG_LENGTH ;

                //process messages

                if(MSG_ID == 50){
                    processMessage50( currentarray );
                }
                else if(MSG_ID == 70){
                    processMessage70( currentarray);
                }


            }


        }






    }

    disIn.close();
    disOut.close();


} catch (IOException e) {
    e.printStackTrace();
}

}
\$\endgroup\$
  • 1
    \$\begingroup\$ You never check for bytecounter > 1023 \$\endgroup\$ – Hogan Apr 17 '14 at 0:37
  • \$\begingroup\$ An occasional blank line to break up long methods is okay; shorter methods that don't need them are better; but two or more in a row is entirely unnecessary. \$\endgroup\$ – David Harkness Apr 17 '14 at 2:25
4
\$\begingroup\$

Some variables, such as socketAlive and hexforlog, aren't being used.

Another quirk I noticed is that you don't really take advantage of the capabilities of DataInputStream. If you just want to read a byte at a time, any InputStream can accomplish that. DataInputStream gives you the ability, for example, to read two bytes and interpret it as an unsigned 16-bit number using .readUnsignedShort(). I'm guessing that that is what your twoBytesToInt() does. Hopefully, the protocol uses big endian numbers so that you can take advantage of .readUnsignedShort().

Your loop is structured such that any 0xfe byte in the input stream will be treated as the end of a packet. That means that no 0xfe byte can ever appear in the payload of a message. Most binary protocols are not designed that way — you should obey the message length fields instead.

Anyway, your code is one long function! It desperately needs some kind of abstraction. bytecounter and all of the concerns surrounding it can go away! The code proposed below compiles, but obviously I can't test it.


Packet.java: This is just a "dumb" struct.

public class Packet {
    public byte som,
                packetClass,    // "class" is a Java keyword
                source,
                dest,
                msgPrgNumber,
                spare1,
                spare2,
                spare3;
    public int  length,         // 2-byte unsigned short
                numMsgs;        // 2-byte unsigned short

    public static class Message {
        public int msgId,
                   length;
        public byte[] payload;
    }
}

PacketInputStream.java: This constructs Packets from the data stream.

import java.io.*;

public class PacketInputStream {

    //////////////////////////////////////////////////////////////////////

    public static class MalformedPacketException extends Exception {
        public MalformedPacketException(String errMsg) {
            super(errMsg);
        }
    }

    //////////////////////////////////////////////////////////////////////

    private DataInputStream in;
    private Packet packet;
    private int messageNumber;

    public PacketInputStream(DataInputStream in) {
        this.in = in;
    }

    /**
     * Reads a 12-byte packet header.
     */
    public Packet readHeader() throws IOException {
        this.packet = new Packet();
        this.messageNumber = 0;
        do {
            this.packet.som       = in.readByte();
        } while (this.packet.som != 0xfd);
        this.packet.packetClass   = in.readByte();
        this.packet.source        = in.readByte();
        this.packet.dest          = in.readByte();
        this.packet.msgPrgNumber  = in.readByte();
        this.packet.spare1        = in.readByte();
        this.packet.spare2        = in.readByte();
        this.packet.spare3        = in.readByte();
        this.packet.length        = in.readUnsignedShort();
        this.packet.numMsgs       = in.readUnsignedShort();
        return this.packet;
    }

    public boolean hasMoreMessages() {
        return this.packet != null && this.messageNumber < this.packet.numMsgs;
    }

    public int getMessageNumber() {
        return this.messageNumber;
    }

    public Packet.Message readMessage() throws IOException {
        Packet.Message msg = new Packet.Message();
        msg.msgId   = in.readUnsignedShort();
        msg.length  = in.readUnsignedShort();
        msg.payload = new byte[msg.length];
        in.read(msg.payload);
        return msg;
    }

    /**
     * Requires the next input byte to be the end-of-packet marker.
     */
    public void readEndOfPacket() throws IOException, MalformedPacketException {
        byte b;
        if ((b = in.readByte()) != 0xfe) {
            throw new MalformedPacketException(String.format("Expected 0xfe, got 0x%02x", b));
        }
    }
}

PacketOutputStream is left as an exercise for the reader.


Consumer:

private void processMessage(Packet.Message msg) {
    switch (msg.msgId) {
      case 50:
        this.processMessage50(msg);
        break;
      case 70:
        this.processMessage70(msg);
        break;
    }
}

public void run() {
    try (DataInputStream  dataIn   = new DataInputStream(in);
         DataOutputStream dataOut  = new DataOutputStream(out)) {
        PacketInputStream  packIn  = new PacketInputStream(dataIn);
        PacketOutputStream packOut = new PacketOutputStream(dataOut);

        while (true) {
            Packet p = packIn.readHeader();

            // Read one message before writing the acknowledgement,
            // because that's what the original code did.  You
            // could simplify this by acknowledging immediately
            // after reading the header instead.
            Packet.Message msg = packIn.readMessage();
            packOut.writeAck(p);
            this.processMessage(msg);

            while (packIn.hasMoreMessages()) {
                this.processMessage(packIn.readMessage());
            }
            packIn.readEndOfPacket();
        }
    } catch (PacketInputStream.MalformedPacketException mpe) {
        mpe.printStackTrace();
    } catch (IOException ioe) {
        ioe.printStackTrace();
    }
}
\$\endgroup\$
2
\$\begingroup\$

Your entire function body is wrapped in a try-catch. That's not good. You should make try blocks as small as possible, ideally containing just the operation itself that might throw.

For example, you could remove the outermost try-catch, and add another catch clause around the while loop that reads into inbyte, something like this:

try {
    // the while loop filling `inbyte`
} catch (EOFException ioef) {
    // handle EOFException
} catch (IOException e) {
    // handle IOException
}

Of course, you will also have to handle dishOut.write and the closing of the streams, and I think you should have separate try-catch blocks for those.


Another example of not putting unnecessary things in a try-catch block:

try {
    disOut.write( java.util.Arrays.copyOfRange(inbyte,0,16) );
    disOut.flush();
} catch (IOException e) { ... }

Since Arrays.copyOfRange(inbyte,0,16) has nothing to do with a potential IOException, it's better to move it out of the try-catch:

byte[] copy = Arrays.copyOfRange(inbyte, 0, 16);
try {
    disOut.write(copy);
    disOut.flush();
} catch (IOException e) { ... }

Formatting issues: you're using blank lines excessively, and the formatting is strange. If you're using Eclipse, see what Control-Shift-f does on selected code, or the equivalent in IntelliJ or Netbeans. It's good to follow the style that these IDEs use.


You have a lot of common coding style violations:

  • Don't use all capitals for local variables. Only constants (static final int SOMECONST = ...) should be named with all caps.
  • You declare many local variables which are not used. If they are used in your real method you should have removed them from the code review.
  • Printing to stdout is a bad practice. Use a Logger instead.

I think it's good to extract constants and declare them near the top of the file, for example these:

private static final byte END_MARKER = (byte) 0xfe;
private static final byte START_MARKER = (byte) 0xfd;
private static final int BUFSIZE = 1024;

One reason is that you can look at the top of the file to see the "magic numbers" the code depends on. Another, if the API changes you can make the changes easily in one place.


As @Hogan pointed out, you never check for bytecount exceeding the boundary of inbyte.

\$\endgroup\$
  • \$\begingroup\$ The key consideration for the try block is not its size, but how you want the flow of control to work when an exception occurs. There's nothing wrong with one big try if you want to bail out of the entire function in the case of an IOException. \$\endgroup\$ – 200_success Apr 17 '14 at 14:20
  • \$\begingroup\$ I first saw this idea in PEP8 (search for the text "Too broad"). Personally I have a more practical reason: if the exception you're catching can come from different actions, it complicates the handling. For example, if there are 2 input streams and the first one throws, then in my catch I have to check if the 2nd is not null before I try to close it. By separating the exception handling, I always know exactly which exception I'm cleaning up. \$\endgroup\$ – janos Apr 17 '14 at 15:02
  • \$\begingroup\$ Unlike IOException, which would only come from communicating with the peer, KeyError could sneak in anywhere. Arguably, if you were expecting a KeyError in a specific place, you would be better of testing if key in collection. \$\endgroup\$ – 200_success Apr 17 '14 at 16:22

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.