Thank you very much!
I'll go through java docs and with the help of your advice will do necessary
changes.
Thanks again,
shumbola
Trustin Lee wrote:
>
> On 5/13/07, shumbola <[EMAIL PROTECTED]> wrote:
>>
>> Hello,
>>
>> Today I've got my feets wet with mina and wrote some code. I've studied
>> examples before that. But I'm not sure if I'm doing things correct, and I
>> kindly ask to review my code. If it is not the way how mina works, point
>> me
>> to the right direction. BTW, I'm using mina 1.1.0. I've wrote a test
>> server
>> and a multithreaded client in java, and was able to communicate in both
>> directions.
>>
>> I'm going to connect to my mina server from the C clients over plain
>> socket
>> calls. From C side, I'll be sending message type, body length and a body
>> as
>> a hexadecimal string (which itself is a complex structure). And expect
>> the
>> same format from server side.
>> In the future I may differentiate Request and Response messages, thats
>> why
>> I've created two distinct classes.
>> So, I've come up with following for now:
>> -------
>> public class RequestMessage implements Serializable {
>> private short type;
>> private String body;
>> }
>> ------
>> public class ResponseMessage implements Serializable {
>> private short type;
>> private String body;
>> }
>> ------
>> public class PaynetPosProtocolCodecFactory extends
>> DemuxingProtocolCodecFactory {
>> public PaynetPosProtocolCodecFactory() {
>> super.register(PosRequestDecoder.class);
>> super.register(PosResponseEncoder.class);
>> }
>> }
>> -----
>> public class ResponseEncoder implements MessageEncoder {
>> private static final Set<Class> TYPES;
>>
>> static
>> {
>> Set<Class> types = new HashSet<Class>();
>> types.add( ResponseMessage.class );
>> TYPES = Collections.unmodifiableSet( types );
>> }
>> public Set<Class> getMessageTypes() {
>> return TYPES;
>> }
>>
>> public void encode(IoSession ioSession, Object message,
>> ProtocolEncoderOutput out) throws Exception {
>> ResponseMessage msg = ( ResponseMessage ) message;
>> ByteBuffer buf = ByteBuffer.allocate( 256 );
>> // Enable auto-expand for easier encoding
>> buf.setAutoExpand( true );
>>
>> try
>> {
>> // output all headers except the content length
>> CharsetEncoder encoder = Charset.defaultCharset()
>> .newEncoder();
>> buf.putShort(msg.getType());
>> buf.putShort((short)msg.getBody().length());
>> buf.putString(msg.getBody(), encoder);
>>
>> }
>> catch( CharacterCodingException ex )
>> {
>> ex.printStackTrace();
>> }
>>
>> buf.flip();
>> out.write( buf );
>> }
>> }
>
> The encoder implementation looks good except that the length field in
> your message can be inaccurate if the body contains non-ascii
> characters. I'd suggest you to use ByteBuffer.putPrefixedString()
> method:
>
> http://mina.apache.org/report/1.0/apidocs/org/apache/mina/common/ByteBuffer.html#putPrefixedString(java.lang.CharSequence,%20java.nio.charset.CharsetEncoder)
>
>> -----
>> public class RequestDecoder extends MessageDecoderAdapter {
>> private short type;
>> private boolean readHeader;
>>
>> public MessageDecoderResult decodable(IoSession ioSession, ByteBuffer
>> in) {
>> // Return NEED_DATA if the whole header is not read yet.
>> if( in.remaining() < Constants.HEADER_LEN )
>> {
>> return MessageDecoderResult.NEED_DATA;
>> }
>> type = in.getShort();
>> if(type >= Constants.TYPE_MIN && type <= Constants.TYPE_MAX)
>> {
>> return MessageDecoderResult.OK;
>> }
>>
>> // Return NOT_OK if not matches.
>> return MessageDecoderResult.NOT_OK;
>> }
>
> decodable() looks OK, too. You've done very good so far! I'd set
> type property in decode() though to make the role of decodable()
> clearer and close to its original intention.
>
>> public MessageDecoderResult decode(IoSession ioSession, ByteBuffer
>> in,
>> ProtocolDecoderOutput out) throws Exception {
>> RequestMessage message = new RequestMessage();
>> message.setType(type);
>>
>> if(!readHeader)
>> {
>> in.getShort();
>> readHeader = true;
>> }
>> if(in.remaining() < 2)
>> {
>> return MessageDecoderResult.NEED_DATA;
>> }
>> short length = in.getShort();
>> if(in.remaining() < length)
>> {
>> return MessageDecoderResult.NEED_DATA;
>> }
>> byte [] buffer = new byte[length];
>> in.get(buffer);
>> String str = new String(buffer);
>> message.setBody(str);
>> out.write(message);
>>
>> readHeader = false;
>>
>> return MessageDecoderResult.OK;
>> }
>> }
>
> This implementation will not work because length field will always be
> read even if it's read already. You did well with the header (type)
> field, but you didn't do anything for the length field. I'd add an
> additional 'readLength' flag. Otherwise, I'd implement in a simpler
> way:
>
> if (in.remaining() < 4) {
> return NEED_DATA;
> }
>
> in.mark();
> short type = in.getShort();
> short length = in.getShort();
>
> if (in.remaining() < length) {
> in.reset();
> return NEED_DATA;
> }
>
> RequestMessage m = ...;
> // TODO read body and set fields...
> out.write(m);
>
> HTH,
> Trustin
> --
> what we call human nature is actually human habit
> --
> http://gleamynode.net/
> --
> PGP Key ID: 0x0255ECA6
>
>
--
View this message in context:
http://www.nabble.com/Help-needed-for-newbie-%28code-review%29-tf3732672.html#a10616880
Sent from the mina dev mailing list archive at Nabble.com.
