You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by cowwoc <co...@bbs.darktech.org> on 2007/07/08 01:25:54 UTC
Refactor ByteBuffer for 2.0?
Hi,
I noticed you are removing pooling and acquire()/release() of ByteBuffer in
2.0. Can you please consider refactoring ByteBuffer to be a helper class
that fits on top of or alongside NIO's ByteBuffer? It seems to me that most
of the methods in your version of ByteBuffer are just convenience methods.
If you look at Grizzly's API they scattered that functionality across
different classes (i.e. ByteBufferInputStream) which led to a much cleaner
API. Right now I feel that ByteBuffer is stuffed full of convenience methods
that most people won't use on a regular basis. I would advocate a minimistic
API containing only methods people are most likely to use because anything
more clutters the API and makes it harder to learn how to use. Also, I don't
mean to offend anyone, but the Javadoc documentation for ByteBuffer needs to
be rewritten by a native English speaker.
Just my 2 cents.
Gili
--
View this message in context: http://www.nabble.com/Refactor-ByteBuffer-for-2.0--tf4042211.html#a11483207
Sent from the mina dev mailing list archive at Nabble.com.
Re: Refactor ByteBuffer for 2.0?
Posted by peter royal <pr...@apache.org>.
On Jul 7, 2007, at 4:25 PM, cowwoc wrote:
> I noticed you are removing pooling and acquire()/release() of
> ByteBuffer in
> 2.0. Can you please consider refactoring ByteBuffer to be a helper
> class
> that fits on top of or alongside NIO's ByteBuffer?
> ...
> Also, I don't mean to offend anyone, but the Javadoc documentation
> for ByteBuffer needs to
> be rewritten by a native English speaker.
Gili, thanks for the feedback..
Care to work up some patches that start on a refactoring and perhaps
clean up the javadocs? :)
-pete
--
proyal@apache.org - http://fotap.org/~osi
Re: Refactor ByteBuffer for 2.0?
Posted by Trustin Lee <tr...@gmail.com>.
Hi,
On 7/8/07, cowwoc <co...@bbs.darktech.org> wrote:
>
> Hi,
>
> I noticed you are removing pooling and acquire()/release() of ByteBuffer in
> 2.0. Can you please consider refactoring ByteBuffer to be a helper class
> that fits on top of or alongside NIO's ByteBuffer? It seems to me that most
> of the methods in your version of ByteBuffer are just convenience methods.
> If you look at Grizzly's API they scattered that functionality across
> different classes (i.e. ByteBufferInputStream) which led to a much cleaner
> API. Right now I feel that ByteBuffer is stuffed full of convenience methods
> that most people won't use on a regular basis. I would advocate a minimistic
> API containing only methods people are most likely to use because anything
> more clutters the API and makes it harder to learn how to use. Also, I don't
> mean to offend anyone, but the Javadoc documentation for ByteBuffer needs to
> be rewritten by a native English speaker.
Which methods are 'most likely to be used'? The bells and whistles we
added are based on our experience, and we found them very useful.
They also follow the naming convention that NIO ByteBuffer used, so I
don't think it doesn't affect learning curve. If there's a
unnecessary method, please let us know.
... and you can always provide us patch if there's broken English in
JavaDoc. As you know, it's easy for other people to find errors than
for the author. Any help is appreciated.
Thanks,
Trustin
--
what we call human nature is actually human habit
--
http://gleamynode.net/
--
PGP Key ID: 0x0255ECA6
Re: Refactor ByteBuffer for 2.0?
Posted by cowwoc <co...@bbs.darktech.org>.
And at the risk of offending even more people: *please* watch this video!
http://www.infoq.com/presentations/effective-api-design
Apache Mina could use a lesson or two from it :)
--
View this message in context: http://www.nabble.com/Refactor-ByteBuffer-for-2.0--tf4042211.html#a11483257
Sent from the mina dev mailing list archive at Nabble.com.
Re: Refactor ByteBuffer for 2.0?
Posted by Trustin Lee <tr...@gmail.com>.
On 7/8/07, cowwoc <co...@bbs.darktech.org> wrote:
> Trustin Lee wrote:
> >
> >> 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.
> >
>
> I was under the impression that direct buffers are being removed in 2.0
> anyway, so this method would go away right?
No. The default buffer type will be changed to non-direct. That's all.
> Trustin Lee wrote:
> >
> > 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.
>
> I'm not sure I understand your point about composition making things
> inconvenient (how?). But besides that a third option is to make the class a
> helper class containing static methods that take in a ByteBuffer per method
> and carry out some operation on it. Such as:
>
> ByteBufferHelper helper = ByteBufferHelper.getInstance();
> helper.fill(buffer, 0);
> System.out.println(helper.getHexValue());
I think it's a matter of preference. I'd rather just call
buffer.fill() and buf.getHexDump().
> You make a good point about the other menthods I mentioned. I'm sorry I
> didn't notice they were already in ByteBuffer. Though I would advocate
> explicitly mentioning that getHexValue() does not modify the position (the
> vast majority of other methods do so it leaves one wondering).
>
>
> Trustin Lee wrote:
> >
> >> 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().
> >
>
> I know it is not your intention but I still find the Javadoc a bit confusing
> here. The getter reads "Returns the current allocator which manages the
> **allocated buffers**." The setter reads "Changes the current allocator with
> the specified one to manage the **allocated buffers from now**."
>
> The existing setter documentation "allocated buffers from now" reads a lot
> like "buffers allocated from now" to me which implies that "from now on all
> new buffers that are created will use this allocator" which probably isn't
> what was meant.
>
> I would recommend using the following Javadoc instead. Getter: "Returns the
> allocator used by existing and new buffers", Setter: "Sets the allocator
> used by existing and new buffers".
I agree with you. Let us work on fixing the documentation.
> In fact if there is only one type of allocator at this time I would advocate
> removing the method altogether. If the user can't do anything meaningful
> with it (yet) then it should not exist and when multiple allocators are
> added in the future you can bring it back.
Sounds like a good idea, but I am still not sure if we are going to
provide only one allocator when we release 2.0. ByteBuffer allocation
is known to affect the performance characteristic of a network
application, so we need to be careful.
Thanks,
Trustin
--
what we call human nature is actually human habit
--
http://gleamynode.net/
--
PGP Key ID: 0x0255ECA6
Re: Refactor ByteBuffer for 2.0?
Posted by cowwoc <co...@bbs.darktech.org>.
Trustin Lee wrote:
>
>> 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.
>
I was under the impression that direct buffers are being removed in 2.0
anyway, so this method would go away right?
Trustin Lee wrote:
>
> 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.
>
I'm not sure I understand your point about composition making things
inconvenient (how?). But besides that a third option is to make the class a
helper class containing static methods that take in a ByteBuffer per method
and carry out some operation on it. Such as:
ByteBufferHelper helper = ByteBufferHelper.getInstance();
helper.fill(buffer, 0);
System.out.println(helper.getHexValue());
etc...
You make a good point about the other menthods I mentioned. I'm sorry I
didn't notice they were already in ByteBuffer. Though I would advocate
explicitly mentioning that getHexValue() does not modify the position (the
vast majority of other methods do so it leaves one wondering).
Trustin Lee wrote:
>
>> 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().
>
I know it is not your intention but I still find the Javadoc a bit confusing
here. The getter reads "Returns the current allocator which manages the
**allocated buffers**." The setter reads "Changes the current allocator with
the specified one to manage the **allocated buffers from now**."
The existing setter documentation "allocated buffers from now" reads a lot
like "buffers allocated from now" to me which implies that "from now on all
new buffers that are created will use this allocator" which probably isn't
what was meant.
I would recommend using the following Javadoc instead. Getter: "Returns the
allocator used by existing and new buffers", Setter: "Sets the allocator
used by existing and new buffers".
In fact if there is only one type of allocator at this time I would advocate
removing the method altogether. If the user can't do anything meaningful
with it (yet) then it should not exist and when multiple allocators are
added in the future you can bring it back.
Gili
--
View this message in context: http://www.nabble.com/Refactor-ByteBuffer-for-2.0--tf4042211.html#a11489137
Sent from the mina dev mailing list archive at Nabble.com.
Re: Refactor ByteBuffer for 2.0?
Posted by Trustin Lee <tr...@gmail.com>.
On 7/8/07, cowwoc <co...@bbs.darktech.org> 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
Re: Refactor ByteBuffer for 2.0?
Posted by cowwoc <co...@bbs.darktech.org>.
Mark Webb-4 wrote:
>
> I really do not think the ByteBuffer javadoc for the trunk is all that
> bad.
> Could you please list some specific areas that would necessitate a
> rewrite?
>
Walking through the API methos in alphabetic order:
1) You should use allocateDirect() instead of allocate(int capacity, boolean
direct) in order to stay in line with NIO's ByteBuffer
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.
3) It isn't clear what happens if you wrap(ByteBuffer) and then expand().
What happens to the underlying ByteBuffer? :)
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.
5) getAllocator() is confusing because it is static. Perhaps you meant to
call it getDefaultAllocator()?
6) getHexDump() doesn't mention any side-effects. Does it modify the buffer
position()?
7) getString(CharsetDecoder) is questionable since
CharsetDecoder.decode(ByteBuffer) already exists
8) getUnsigned() should be renamed to getUnsignedShort() keeping in line
with getUnsignedInt().
9) mark(), limit() are also questionable. They should be hidden inside the
InputStream view of the ByteBuffer, not exposed on its main API.
10) prefixedDataAvailable()'s Javadoc reads "Returns true if this buffer
contains a data which has a data length as a prefix and the buffer has
remaining data as enough as specified in the data length field". I don't
know about you, but I have no clue what that means :) Either there is a typo
or something is wrong with the sentence structure, either way I don't
understand sorry :(
11) putChar() and all String related operations are already provided by
ByteBuffer.asCharBuffer()'s API.
12) It isn't exactly clear how getObject(), putObject() works. Does the
Object have to be Serializable?
13) Sweep()'s Javadoc talks about clearing the buffer, perhaps this method
should be renamed to clear()?
14) It isn't clear what ByteBuffer.toString() does. The Javadoc is empty.
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
It is an hour long, but it is worth every second of your time!
Gili
--
View this message in context: http://www.nabble.com/Refactor-ByteBuffer-for-2.0--tf4042211.html#a11486146
Sent from the mina dev mailing list archive at Nabble.com.
Re: Refactor ByteBuffer for 2.0?
Posted by Mark <el...@gmail.com>.
I really do not think the ByteBuffer javadoc for the trunk is all that bad.
Could you please list some specific areas that would necessitate a rewrite?
Thank you.
On 7/7/07, cowwoc <co...@bbs.darktech.org> wrote:
>
>
> Hi,
>
> I noticed you are removing pooling and acquire()/release() of ByteBuffer
> in
> 2.0. Can you please consider refactoring ByteBuffer to be a helper class
> that fits on top of or alongside NIO's ByteBuffer? It seems to me that
> most
> of the methods in your version of ByteBuffer are just convenience methods.
> If you look at Grizzly's API they scattered that functionality across
> different classes (i.e. ByteBufferInputStream) which led to a much cleaner
> API. Right now I feel that ByteBuffer is stuffed full of convenience
> methods
> that most people won't use on a regular basis. I would advocate a
> minimistic
> API containing only methods people are most likely to use because anything
> more clutters the API and makes it harder to learn how to use. Also, I
> don't
> mean to offend anyone, but the Javadoc documentation for ByteBuffer needs
> to
> be rewritten by a native English speaker.
>
> Just my 2 cents.
>
> Gili
> --
> View this message in context:
> http://www.nabble.com/Refactor-ByteBuffer-for-2.0--tf4042211.html#a11483207
> Sent from the mina dev mailing list archive at Nabble.com.
>
>
--
..Cheers
Mark