I don't like your refactoring .. I will explain inline why ;-)
Ok!
[EMAIL PROTECTED] schrieb:
Author: bago
Date: Thu Nov 16 14:22:58 2006
New Revision: 475953
URL: http://svn.apache.org/viewvc?view=rev&rev=475953
Log:
Minor refactoring for AbstractActionHandler (JAMES-614)
Modified:
james/server/trunk/src/java/org/apache/james/smtpserver/core/filter/fastfail/AbstractActionHandler.java
james/server/trunk/src/java/org/apache/james/smtpserver/core/filter/fastfail/MaxRcptHandler.java
james/server/trunk/src/java/org/apache/james/smtpserver/core/filter/fastfail/ValidSenderDomainHandler.java
Modified:
james/server/trunk/src/java/org/apache/james/smtpserver/core/filter/fastfail/AbstractActionHandler.java
URL:
http://svn.apache.org/viewvc/james/server/trunk/src/java/org/apache/james/smtpserver/core/filter/fastfail/AbstractActionHandler.java?view=diff&rev=475953&r1=475952&r2=475953
==============================================================================
---
james/server/trunk/src/java/org/apache/james/smtpserver/core/filter/fastfail/AbstractActionHandler.java
(original)
+++
james/server/trunk/src/java/org/apache/james/smtpserver/core/filter/fastfail/AbstractActionHandler.java
Thu Nov 16 14:22:58 2006
@@ -93,14 +93,14 @@
}
/**
- * Process the checking
- *
- * @param session the SMTPSession
+ * @see org.apache.james.smtpserver.CommandHandler#onCommand(SMTPSession)
*/
- protected void doProcessing(SMTPSession session) {
+ public void onCommand(SMTPSession session) {
if (check(session)) {
if (getAction().equals(JunkScoreConfigUtil.JUNKSCORE)) {
- getLogger().info(getJunkScoreLogString(session));
+ if (getLogger().isInfoEnabled()) {
+ getLogger().info(getJunkScoreLogString(session)+" Add
Junkscore: " + getScore());
+ }
JunkScore junk = getJunkScore(session);
junk.setStoredScore(getScoreName(), getScore());
I used the method doProcessing cause it will be also possible to extend
this class in a ConnectHandler or a MessageHandler... IMHO this makes
more sense..
You can override the onCommand at the same way you override the
doProcessing. I don't understand what you gain from adding a further
call, but this is really a minor issue.
@@ -157,5 +157,7 @@
*
* @return junkScore
*/
- protected abstract JunkScore getJunkScore(SMTPSession session);
+ protected JunkScore getJunkScore(SMTPSession session) {
+ return (JunkScore) session.getState().get(JunkScore.JUNK_SCORE);
+ }
}
I keept it abstract course we must be able to choose if we want to store
the scores in the junkScore which is valid per mail and the one which is
valid for the whole session. Now you hardcode it to the junkScore which
is valid for the mail. This will make no sense if we use a junkscore in
DNSRBLHandler. For this class it whould be:
protected JunkScore getJunkScore(SMTPSession session) {
return (JunkScore)
session.getConnectionState().get(JunkScore.JUNK_SCORE);
}
You're right on the connection/message state maps, but you can have a
default in the Abstract and override it only when needed.
Otherwise a good solution could be to have a method that return the
correct state map and then leave to the Abstract class the duty to
get/set the JunkScore in that class. Otherwise we could put a method to
return a flag to say if the handler operates on the session or on the
message state: I just don't like to see "Junk" stuff in the specializations.
Either way, this refactoring are so minor that feel free to revert my
changes while you work on the remaining handlers and then we'll have a
better overview on the possible refactorings.
Stefano
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
