mina-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From shumbola <shumbola2...@yahoo.com>
Subject Re: Help needed for newbie (code review)
Date Tue, 15 May 2007 05:41:08 GMT

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 <shumbola2004@yahoo.com> 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.


Mime
View raw message