On 7/8/07, cowwoc <[EMAIL PROTECTED]> wrote:
Walking through the API methos in alphabetic order:
I won't answer for documentation-related issues. We already know we need a lot of work there.
1) You should use allocateDirect() instead of allocate(int capacity, boolean direct) in order to stay in line with NIO's ByteBuffer
I chose to use a boolean parameter because it is much easier for a user to choose the type of buffer in runtime (i.e. programatically). It is trivial to add allocateDirect of course, but I don't think it's really necessary.
2) array(), arrayOffset(), asCharBuffer(), and many others are missing Javadoc. All they contain is a hyperlink back to NIO.ByteBuffer. You should either subclass ByteBuffer or use composition. If you go with composition you can remove all methods which already exist in ByteBuffer and just add a single getBuffer() method which returns the underlying buffer. Either case takes care of documentation and exposing all ByteBuffer-related functionality.
I already tried subclassing, but NIO ByteBuffer doesn't allow subclassing effectively. Composition is neither a good idea because it will make the manipulation of the buffer content inconvenient. We found all those new getters and putters are useful when we deal with various proprietary binary protocols.
4) I would eliminate fill(int), fillAndReset(int) or rename them to clear(int), clearAndReset(). Consider the cost/benefit of adding convinience methods for a special value of zero. I would just force the user to pass in the zero in favor of dropping two methods. Also the fact that fill(byte, int) takes two numbers in a row is dangerous; the user should ideally get a compiler error if he passes in the arguments in the wrong order.
NIO Buffer already has a method called 'clear', and that's why I chose another name. fill(byte, int) will give you the correct compilation error unless you specified two byte parameters, which is not likely to happen.
5) getAllocator() is confusing because it is static. Perhaps you meant to call it getDefaultAllocator()?
Property name 'defaultAllocator' implies that you can choose an alternative allocator in runtime. You can not actually and there is only one allocator you can use from ByteBuffer.allocate().
6) getHexDump() doesn't mention any side-effects. Does it modify the buffer position()?
It doesn't mention any side effect because it does not modify the buffer position.
7) getString(CharsetDecoder) is questionable since CharsetDecoder.decode(ByteBuffer) already exists
CharsetDecoder.decode(ByteBuffer) doesn't detect NUL (0x00), which is often used as a string terminator in various binary protocols.
8) getUnsigned() should be renamed to getUnsignedShort() keeping in line with getUnsignedInt().
No. getUnsigned() doesn't read a short integer but a byte. It returns a short integer to express a unsigned byte.
9) mark(), limit() are also questionable. They should be hidden inside the InputStream view of the ByteBuffer, not exposed on its main API.
mark() and limit() are also public in NIO ByteBuffer.
11) putChar() and all String related operations are already provided by ByteBuffer.asCharBuffer()'s API.
putChar() and getChar() are also provided by NIO ByteBuffer. Most protocol implementors don't work with a CharBuffer but directly with a ByteBuffer.
13) Sweep()'s Javadoc talks about clearing the buffer, perhaps this method should be renamed to clear()?
NIO ByteBuffer already got clear().
There are many more small nuances but those are the major ones that come to mind right now. On a related note, if any of you haven't seen this video yet, I *highly* recommend it: http://www.infoq.com/presentations/effective-api-design
I'm also a great admirer of nice and effective API design philosophy he urges us. I am sorry, but I doubt you don't have any serious experience in implementing various protocols, nor even with a simple ByteBuffer manipulation IMHO. I admit we have problems with documentation. Except that, I don't think our ByteBuffer API violates what the principle of API design is for; customers. Thank you anyway for pointing out various potential issues of MINA. We always review our code whenever any issue arises. Trustin -- what we call human nature is actually human habit -- http://gleamynode.net/ -- PGP Key ID: 0x0255ECA6
