stefano, thanks for the feedback. find my responses inline
-- Anagha
On 8/17/05, Stefano Bagnara <[EMAIL PROTECTED]> wrote:
> A few notes/questions from a first review:
>
> 1) Your tipical onCommand implementation is:
>
> public void onCommand(SMTPSession session) {
> //deviation from the Main code
> //Instead of throwing exception just end the session
> try{
> doAUTH(session, session.getCommandArgument());
> } catch (Exception ex) {
> session.getLogger().error("Exception occured:" +
> ex.getMessage());
> session.endSession();
> }
> }
>
> You catch "Exception" and return a generic error closing the session.
> I'm not sure but IIRC the original code didn't catch the exception in every
> command but has specific catch outside.
> It could be probably better to add the possible exception to the
> CommandHandler / ConnectHandler interfaces and add the "throws" in the
> onCommand/onConnect so you don't have to catch the generic exception while
> handling the command.
> The main thing I don't like is the "catch(Exception ex)" that will catch
> EVERY problem and hide the whole stacktrace/cause.
>
None of the commandhandlers throw exception except doAuth().
Even the doData also handles the exception internally
I was also planning to make onCommand throw exception. If so should
it should the generic Exception. Should I throw the base class Exception?
> 2) Why do we need a "private final static String COMMAND_NAME" inside every
> command handler? "Is it"/"Will it be" used? The mapping between command name
> and command handler is done at configuration level so I don't understand the
> meaning of that private string.
When I refactored all the commandhandlers I renamed the command name
constants to COMMAND_NAME. Initially I was planning to use the
COMMAND_NAME for getting the right commandhandler. But I moved it to
XML configuration. So as of now it doesnot have any significance. I
will remove it
wherever its not used.
>
> 3) Why did you choose to create a private inner class SMTPSessionImpl
> implementing the SMTPSession instead of make the SMTPHandler to implement
> the interface itself?
The reason for that is that I dont want the 3rd party command handler
writers to cast SMTPSession to SMTPHandler and access the
functionality. If you think it makes the design inefficient i will do
as you advised
>
> 4) Why did you choose to create the following states?:
> private final static byte COMMAND_MODE = 1;
> private final static byte RESPONSE_MODE = 2;
> private final static byte MESSAGE_RECEIVED_MODE = 3;
> private final static byte MESSAGE_ABORT_MODE = 4;
> Can you explain the meaning of the states and their usage?
> In particular I'm interested to know why do you need to add the
> RESPONSE_MODE ?
>
At the beginning
mode = command_mode
once the commandHandler writes response, the mode is changed to
RESPONSE_MODE. This will cause to skip the next command handlers
configured for that command.
For instance:
There are 2 commandhandlers MailAddressChecker and MailCmdHandler for
MAIL command. If MailAddressChecker validation of the MAIL FROM
address is successful, the MailCmdHandlers will be executed.
Incase it fails, it has to write response. Once we write response
there is no need to execute the MailCmdHandler.
Thus RESPONSE_MODE is used to skip execution of remaining
commandhandlers for that command
Next, Once MAIL message is received the DataCmdHandler and any other
equivalent commandHandler will call setMail method.
this will change the mode to MAIL_RECEIVED_MODE
This mode will trigger the message handlers to be execute. Message handlers
can abort message. In that message will not spooled.
All the handlers can abort session, in that case it will get out of
the main loop
and session will end.
> 5) I've not tested it but I'm not sure you handle the RSET correctly. I
> don't see anywhere the assignment of mode = COMMAND_MODE when you reset the
> session (after an RSET).
At the beginning its reset back to command mode
while(!sessionEnded) {
//Reset the current command values
curCommandName = null;
curCommandArgument = null;
mode = COMMAND_MODE; <--------------
I have tested RSET it works fine.
>
> Stefano
>
> PS: I didn't run the code, only merged it with my code and read it.
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [EMAIL PROTECTED]
> For additional commands, e-mail: [EMAIL PROTECTED]
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
