Wrapping sockets for bidirectional use



  • I came across this snippet in a 6-month old system - apparently some rookie didn't know that a socket can be used to send data in both directions. Interestingly, the person correctly handled partial buffers arriving...

    import java.net.Socket;
    import java.io.IOException;
    import java.text.DecimalFormat;

    // WTF # 1 - do we really need to wrap sockets to encapsulate bidirectional communications?
    public class Pipe {
    private Socket inbound; // WTF # 2 - actually using two sockets to implement round trip communications
    private Socket outbound; // instead of using input AND output channels

    // WTF # 3 - technically, these args should be 'final', minor wtf
    public Pipe(String remoteIP, int remotePort) {
    	// WTF # 4 - which connection failed: outbound or inbound?
    	try {
    		outbound = openSocket(remoteIP, remotePort);
    		inbound = openSocket(remoteIP, remotePort);
    	}
    	catch (Exception e) {
    		// WTF # 5 - what was the actual error, other than something went wrong?
    		System.err.println("Unable to establish round trip communications");
    	}
    }
    
    // WTF # 6 - why create a proc for one line of code - arguably minor wtf -
    //           could be for encapsulation (which would be a valid use), but
    //           in this case I don't think so; also args should be final (minor gripe)
    private Socket openSocket(String remoteIP, int remotePort) throws IOException {
    	return new Socket(remoteIP, remotePort);
    }
    
    // WTF # 7 - padding involves duplicating buffer storage instead of sending just a 'length' buf first (see below)
    public void sendOutboundMessage(byte data[]) throws IOException {
    	byte tmp[] = padBufferWithMessageLength(data);
    	outbound.getOutputStream().write(tmp);
    	outbound.getOutputStream().flush();
    }
    
    // Actually, this is not too bad - it deals with partial buffer reads
    // WTF # 8 - magic constants (bufLen field size = 6)
    public byte [] getInboundMessage() throws IOException {
    	byte msgLen[] = new byte[6];
    	for (int i=0; i<6; i++) {
    		msgLen[i] = (byte)inbound.getInputStream().read();
    	}
    	int numBytesToRead = Integer.parseInt(new String(msgLen));
    	int offset = 0;
    	byte data[] = new byte[numBytesToRead];
    	while (numBytesToRead > 0) {
    		int nRead = inbound.getInputStream().read(data,offset,numBytesToRead);
    		numBytesToRead -= nRead;
    		offset += nRead;
    	}
    	return data;
    }
    
    // WTF # 9 - why duplicate the entire buffer in order to prefix a buf-length
    //           (why not just send one short buffer with buf length, then the main
    //           data buffer afterward, without duplicating storage?)
    private byte [] padBufferWithMessageLength(byte b[]) {
    	byte tmp[] = new byte[b.length + 6];
    	// WTF # 10 - why not use a 4 byte int up front to send the message length - gives 2 billion byte messages
    	//            - instead of a 6 byte String, with associated conversions
    	DecimalFormat df = new DecimalFormat("000000");
    	String lenStr = df.format(b.length);
    	System.arraycopy(lenStr.getBytes(),0,tmp,0,6);
    	System.arraycopy(b,0,tmp,6,b.length);
    	return tmp;
    }
    

    }



  • #3: I'm not sure I agree.  A 'final' parameter or or local
    variable is given scope beyond the life of the method, which is not the
    same as what 'const' does in C.  As far as I know, the Java
    Virtual Machine Specification doesn't address exactly how long a local
    final variable lives, or where it's stored, but it's probably safe to
    assume that the variable does not go on the method's stack.  Thus,
    I don't make parameters or local variables final unless I need to.



    #5:  I assume you're referring to "catch Exception" which is definitely an abomination.



    #6:  Like you, I would think this is for encapsulation.  The
    most obvious possibility is the desire in the future to set some socket
    options (SO_LINGER, etc.).



    I'm with you on all the other WTFs.



  • @he-she said:

    <FONT face="Courier New">// WTF # 3 - technically, these args should be 'final', minor wtf
    public Pipe(String remoteIP, int remotePort) {
    </FONT>

    Why?  The only reason I know of to make them final would be to access them from an inner class.  I've certainly never seen anyone use final arguments for any other reason, although I admit my experience of Java, or anything else for that matter, In The Enterprise™ is pretty non-existant.



  • @VGR said:



    #5:  I assume you're referring to "catch Exception" which is definitely an abomination.


    Or maybe the fact that nothing about the exception is printed, ala e.printStackTrace()...



  • @endergt said:

    @VGR said:


    #5:  I assume you're referring to "catch Exception" which is definitely an abomination.


    Or maybe the fact that nothing about the exception is printed, ala e.printStackTrace()...


    Very true.  I was so appalled by "catch Exception" that I didn't
    even think to look inside the block.  printStackTrace() is a
    million times better than just printing a miscellaneous "something went
    wrong" message.  (Not that printStackTrace() is acceptable error
    handling, but it's still significantly better than a mere
    println.)  Another great example of a programmer who doesn't know
    what exceptions are for.



  • #9: to reduce the number of calls to the native layer and to try to convince each message to line up with a TCP packet boundary to make life that much easier on the receiving end. Duplicating the data isn't a WTF, but rolling your own message buffering rather than using an existing version of that wheel, like BufferedOutputStream, is a bit silly.

    You missed #10: using String#getBytes() to encode a string.  It's "probably" safe for a string containing only digits, but getBytes depends on the current default encoding, which may not be compatible between systems.


Log in to reply
 

Looks like your connection to What the Daily WTF? was lost, please wait while we try to reconnect.