Wrapping sockets for bidirectional use

he_she

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&lt;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 &gt; 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;
}

}

VGR

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

iwpg

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

endergt

@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()...

VGR

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

Angstrom

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