You are viewing a plain text version of this content. The canonical link for it is here.
Posted to server-dev@james.apache.org by "Noel J. Bergman" <no...@devtech.com> on 2002/08/04 05:58:37 UTC
String.equalsIgnoreCase Considered Evil :-)
There are many places in James where we have lengthy sets of comparisons of
an unknown string value to a known string value. These are many of these of
the form:
string.equalsIgnoreCase(<literal>)
This use is terribly inefficient. Each and every call iterates through both
the string and the literal, uppercasing both as it compares them.
Please, as you are working on code (Peter will apply these to POP3Handler
and SMTPHandler), if you see this pattern, please change it to:
string = string.to[Upper|Lower]Case(); // chose depending upon your
literals
string.equals(<literal>)
For example, in SMTPHandler and POP3Handler:
String command = commandRaw.trim();
becomes
String command = commandRaw.trim().toUpperCase();
and the test for "USER" (for example) becomes:
if (command.equals("USER")) ...
Actually, I believe that we should add a command-map model to the handlers,
but that's a seperate issue for a separate thread. The change proposed in
this e-mail is simple.
--- Noel
P.S. Brownie points for whomever recognizes the origin of the subject
header
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: String.equalsIgnoreCase Considered Evil :-)
Posted by Harmeet Bedi <ha...@kodemuse.com>.
v1.150. As the attached
> program illustrates,
Very impressive turnaround. :-)
I had always tool it for granted that <equalsIgnoreCase> pattern is better.
There is the additional object creation with toUpperCase method. Minor thing
though... I think the changes are good.
Harmeet
----- Original Message -----
From: "Noel J. Bergman" <no...@devtech.com>
To: "James Developers List" <ja...@jakarta.apache.org>
Sent: Monday, August 05, 2002 1:06 PM
Subject: RE: String.equalsIgnoreCase Considered Evil :-)
>
> I am running with JDK 1.4, which has String.java v1.150. As the attached
> program illustrates, using String.equalsIgnoreCase is faster than using
> String.equals for the first command on the chain, about break-even for the
> second, and generally slower for all other commands.
>
> One reason for this is that String.toUpperCase is optimized. In the
normal
> event that the SMTP client passes upper case commands across the wire,
> String.toUpperCase simply returns the original string.
>
> However, even in the case where the command is passed in lower case, the
> result is simply shifted a couple of comparisons down the chain. And
since
> we have not sorted the command chain by frequency of use, commands such as
> DATA and RETR are slower with the original code. When we get to handlers
> such as IMAP which have a more extensive and active set of commands, the
> impact will be even greater. Oh, and there is also an impact based upon
> command length, which is why TOP is faster with equalsIgnoreCase, even
> though it is further down the chain. On the other hand, longer IMAP
> commands will be slower.
>
> None of this is about big chunks of time, but the change was simple
enough,
> and is demonstrably faster.
>
> --- Noel
>
----------------------------------------------------------------------------
----
> --
> To unsubscribe, e-mail:
<ma...@jakarta.apache.org>
> For additional commands, e-mail:
<ma...@jakarta.apache.org>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: String.equalsIgnoreCase Considered Evil :-)
Posted by "Noel J. Bergman" <no...@devtech.com>.
I am running with JDK 1.4, which has String.java v1.150. As the attached
program illustrates, using String.equalsIgnoreCase is faster than using
String.equals for the first command on the chain, about break-even for the
second, and generally slower for all other commands.
One reason for this is that String.toUpperCase is optimized. In the normal
event that the SMTP client passes upper case commands across the wire,
String.toUpperCase simply returns the original string.
However, even in the case where the command is passed in lower case, the
result is simply shifted a couple of comparisons down the chain. And since
we have not sorted the command chain by frequency of use, commands such as
DATA and RETR are slower with the original code. When we get to handlers
such as IMAP which have a more extensive and active set of commands, the
impact will be even greater. Oh, and there is also an impact based upon
command length, which is why TOP is faster with equalsIgnoreCase, even
though it is further down the chain. On the other hand, longer IMAP
commands will be slower.
None of this is about big chunks of time, but the change was simple enough,
and is demonstrably faster.
--- Noel
Re: String.equalsIgnoreCase Considered Evil :-)
Posted by Harmeet Bedi <ha...@kodemuse.com>.
> string.equalsIgnoreCase(<literal>)
>
> This use is terribly inefficient. Each and every call iterates through
both
> the string and the literal, uppercasing both as it compares them.
No only till there is a match. The conversion would cease if a character
does not match.
Alternatively
String command = command.toUpperCase()
command.equals(<somecmd>)
would always convert the character to upper case even if there the command
string does not have a single character common with <somecmd>
In most cases the character conversion will happen for only one character.
That should be enough to reject the comparison
I think it would be better to find hotspots by profiling. The code may look
inefficient but may be fine. There may be other place like MailImpl, Linear
processor etc. where optimization may yield very good results.
Harmeet
----- Original Message -----
From: "Noel J. Bergman" <no...@devtech.com>
To: "James-Dev Mailing List" <ja...@jakarta.apache.org>
Sent: Saturday, August 03, 2002 8:58 PM
Subject: String.equalsIgnoreCase Considered Evil :-)
> There are many places in James where we have lengthy sets of comparisons
of
> an unknown string value to a known string value. These are many of these
of
> the form:
>
> string.equalsIgnoreCase(<literal>)
>
> This use is terribly inefficient. Each and every call iterates through
both
> the string and the literal, uppercasing both as it compares them.
>
> Please, as you are working on code (Peter will apply these to POP3Handler
> and SMTPHandler), if you see this pattern, please change it to:
>
> string = string.to[Upper|Lower]Case(); // chose depending upon your
> literals
>
> string.equals(<literal>)
>
> For example, in SMTPHandler and POP3Handler:
>
> String command = commandRaw.trim();
> becomes
> String command = commandRaw.trim().toUpperCase();
>
> and the test for "USER" (for example) becomes:
>
> if (command.equals("USER")) ...
>
> Actually, I believe that we should add a command-map model to the
handlers,
> but that's a seperate issue for a separate thread. The change proposed in
> this e-mail is simple.
>
> --- Noel
>
> P.S. Brownie points for whomever recognizes the origin of the subject
> header
>
>
> --
> To unsubscribe, e-mail:
<ma...@jakarta.apache.org>
> For additional commands, e-mail:
<ma...@jakarta.apache.org>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] POP3Handler.java v.2
Posted by Harmeet Bedi <ha...@kodemuse.com>.
> It's an interesting idea, although there's at least one serious problem
> with this suggestion. In short, not every response goes to the log. If
> we tried to merge the output stream with the logging stream then we'd be
> sending all data (returned messages, etc.) to the log, which is not what
> we want to do. What we want to do is record the
Agreed, good point. Missed that one. In the past I had 2 methods to do
something like this - <printlnAndLog> and <println> The main advantage
I found was that there was no need to write println and log methods
separately. No big deal. the only gain is one method call instead of
two.
Harmeet
----- Original Message -----
From: "Peter M. Goldstein" <pe...@yahoo.com>
To: "'James Developers List'" <ja...@jakarta.apache.org>
Sent: Monday, August 05, 2002 11:23 AM
Subject: RE: [PATCH] POP3Handler.java v.2
>
> Harmeet,
>
> > Feedback 1
> > ==========
> > instead of doing
> > out.println(...)
> > logResponseString(...)
> >
> > Would it better to override <println> to log ?
> > At the time of PrintWriter creation in <compose>, log level could be
> > checked
> > and a Loggable PrintWriter created. This may be cleaner and may avoid
> > multiple calls to <logResponseString> and check for
> > if (getLogger().isDebugEnabled())
> > in <logResponseString>
>
> It's an interesting idea, although there's at least one serious problem
> with this suggestion. In short, not every response goes to the log. If
> we tried to merge the output stream with the logging stream then we'd be
> sending all data (returned messages, etc.) to the log, which is not what
> we want to do. What we want to do is record the commands sent back, not
> the entirety of the data sent back to the client. Keeping the writers
> separate is a result of this requirement.
>
> I'm also not nuts about caching the log level. The reason for this is
> that I don't see anywhere in the Logger contract a guarantee that the
> log level will remain unchanged for the life of the server. I can
> certainly think of a number of situations where I'd like to be able to
> change the log level midstream, and I suspect that at least one future
> logging implementation will allow that behavior. The cost of what, in
> the overwhelming majority of cases, will be calling a method that just
> returns a Boolean cached in the logger is fairly minimal. If I'm wrong
> about the Logger contract I'd be happy to implement a variant of this
> optimization. If not, I value obeying the contract over a minor
> performance enhancement.
>
> --Peter
>
>
>
> --
> To unsubscribe, e-mail:
<ma...@jakarta.apache.org>
> For additional commands, e-mail:
<ma...@jakarta.apache.org>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: [PATCH] POP3Handler.java v.2
Posted by "Peter M. Goldstein" <pe...@yahoo.com>.
Harmeet,
> Feedback 1
> ==========
> instead of doing
> out.println(...)
> logResponseString(...)
>
> Would it better to override <println> to log ?
> At the time of PrintWriter creation in <compose>, log level could be
> checked
> and a Loggable PrintWriter created. This may be cleaner and may avoid
> multiple calls to <logResponseString> and check for
> if (getLogger().isDebugEnabled())
> in <logResponseString>
It's an interesting idea, although there's at least one serious problem
with this suggestion. In short, not every response goes to the log. If
we tried to merge the output stream with the logging stream then we'd be
sending all data (returned messages, etc.) to the log, which is not what
we want to do. What we want to do is record the commands sent back, not
the entirety of the data sent back to the client. Keeping the writers
separate is a result of this requirement.
I'm also not nuts about caching the log level. The reason for this is
that I don't see anywhere in the Logger contract a guarantee that the
log level will remain unchanged for the life of the server. I can
certainly think of a number of situations where I'd like to be able to
change the log level midstream, and I suspect that at least one future
logging implementation will allow that behavior. The cost of what, in
the overwhelming majority of cases, will be calling a method that just
returns a Boolean cached in the logger is fairly minimal. If I'm wrong
about the Logger contract I'd be happy to implement a variant of this
optimization. If not, I value obeying the contract over a minor
performance enhancement.
--Peter
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] POP3Handler.java v.2
Posted by Harmeet Bedi <ha...@kodemuse.com>.
Feedback 1
==========
instead of doing
out.println(...)
logResponseString(...)
Would it better to override <println> to log ?
At the time of PrintWriter creation in <compose>, log level could be checked
and a Loggable PrintWriter created. This may be cleaner and may avoid
multiple calls to <logResponseString> and check for
if (getLogger().isDebugEnabled())
in <logResponseString>
BTW. final is redundant in <logResponseString>
Feedback 2
==========
command = commandLine.nextToken().toUpperCase();
....
return (command.equals("QUIT") == false);
is slower than
command = commandLine.nextToken();
....
return (command.equalsIgnoreCase("QUIT") == false);
The reason is that latter pattern does not create a new String object
and the former does by calling <String::toUpperCase()>. In one of the
JDK versions(1.3?) <String::equalsIgnoreCase> optimizations were added
to avoid object creation.
Harmeet
----- Original Message -----
From: "Peter M. Goldstein" <pe...@yahoo.com>
To: "'James Developers List'" <ja...@jakarta.apache.org>
Sent: Sunday, August 04, 2002 10:39 AM
Subject: [PATCH] POP3Handler.java v.2
>
> All,
>
> Attached is a patch to the POP3Handler.java code that rolls in the
> String.equalsIgnoreCase change that Noel suggested.
>
> All other previously made changes were left intact.
>
> Attached is the source and diff. As always, comments, criticisms, and
> questions are welcome.
>
> --Peter
>
> > -----Original Message-----
> > From: Noel J. Bergman [mailto:noel@devtech.com]
> > Sent: Saturday, August 03, 2002 8:59 PM
> > To: James-Dev Mailing List
> > Subject: String.equalsIgnoreCase Considered Evil :-)
> >
> > There are many places in James where we have lengthy sets of
> comparisons
> > of
> > an unknown string value to a known string value. These are many of
> these
> > of
> > the form:
> >
> > string.equalsIgnoreCase(<literal>)
> >
> > This use is terribly inefficient. Each and every call iterates
> through
> > both
> > the string and the literal, uppercasing both as it compares them.
> >
> > Please, as you are working on code (Peter will apply these to
> POP3Handler
> > and SMTPHandler), if you see this pattern, please change it to:
> >
> > string = string.to[Upper|Lower]Case(); // chose depending upon
> your
> > literals
> >
> > string.equals(<literal>)
> >
> > For example, in SMTPHandler and POP3Handler:
> >
> > String command = commandRaw.trim();
> > becomes
> > String command = commandRaw.trim().toUpperCase();
> >
> > and the test for "USER" (for example) becomes:
> >
> > if (command.equals("USER")) ...
> >
> > Actually, I believe that we should add a command-map model to the
> > handlers,
> > but that's a seperate issue for a separate thread. The change
> proposed in
> > this e-mail is simple.
> >
> > --- Noel
> >
> > P.S. Brownie points for whomever recognizes the origin of the subject
> > header
> >
> >
> > --
> > To unsubscribe, e-mail: <mailto:james-dev-
> > unsubscribe@jakarta.apache.org>
> > For additional commands, e-mail: <mailto:james-dev-
> > help@jakarta.apache.org>
>
----------------------------------------------------------------------------
----
> --
> To unsubscribe, e-mail:
<ma...@jakarta.apache.org>
> For additional commands, e-mail:
<ma...@jakarta.apache.org>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
[PATCH] POP3Handler.java v.2
Posted by "Peter M. Goldstein" <pe...@yahoo.com>.
All,
Attached is a patch to the POP3Handler.java code that rolls in the
String.equalsIgnoreCase change that Noel suggested.
All other previously made changes were left intact.
Attached is the source and diff. As always, comments, criticisms, and
questions are welcome.
--Peter
> -----Original Message-----
> From: Noel J. Bergman [mailto:noel@devtech.com]
> Sent: Saturday, August 03, 2002 8:59 PM
> To: James-Dev Mailing List
> Subject: String.equalsIgnoreCase Considered Evil :-)
>
> There are many places in James where we have lengthy sets of
comparisons
> of
> an unknown string value to a known string value. These are many of
these
> of
> the form:
>
> string.equalsIgnoreCase(<literal>)
>
> This use is terribly inefficient. Each and every call iterates
through
> both
> the string and the literal, uppercasing both as it compares them.
>
> Please, as you are working on code (Peter will apply these to
POP3Handler
> and SMTPHandler), if you see this pattern, please change it to:
>
> string = string.to[Upper|Lower]Case(); // chose depending upon
your
> literals
>
> string.equals(<literal>)
>
> For example, in SMTPHandler and POP3Handler:
>
> String command = commandRaw.trim();
> becomes
> String command = commandRaw.trim().toUpperCase();
>
> and the test for "USER" (for example) becomes:
>
> if (command.equals("USER")) ...
>
> Actually, I believe that we should add a command-map model to the
> handlers,
> but that's a seperate issue for a separate thread. The change
proposed in
> this e-mail is simple.
>
> --- Noel
>
> P.S. Brownie points for whomever recognizes the origin of the subject
> header
>
>
> --
> To unsubscribe, e-mail: <mailto:james-dev-
> unsubscribe@jakarta.apache.org>
> For additional commands, e-mail: <mailto:james-dev-
> help@jakarta.apache.org>
Re: [PATCH] SMTPHandler.java v.2 - SMTP ID, Random.nextInt
Posted by Harmeet Bedi <ha...@kodemuse.com>.
I was pointing out that the entire smtp id code is unnecessary. Removing
unnecessary code may be a good optimization.
smtpID variable and SMTP_ID state could be removed, so there would be no
need to optimize or look at it.
Harmeet
----- Original Message -----
From: "Peter M. Goldstein" <pe...@yahoo.com>
To: "'James Developers List'" <ja...@jakarta.apache.org>
Sent: Monday, August 05, 2002 11:26 AM
Subject: RE: [PATCH] SMTPHandler.java v.2 - SMTP ID, Random.nextInt
>
> Harmeet,
>
> I just don't get your objection.
>
> SMTP ID does one thing and one thing only - allows you to identify from
> the external SMTP exchange which connection/thread it was associated
> with. It's not a tremendously useful feature I grant you, but in the
> case of an extreme fatal error (one that resulted in a thread dump) it
> may very well allow you to connection the external SMTP connection with
> the single thread among potentially hundreds that was connecting to a
> particular SMTP exchange. Presumably it was added to the code base for
> precisely that reason. I'm reluctant to change an exposed, functioning
> piece of code from the software without any sort of compelling reason.
>
> The cost of this feature is negligible. The Random is now shared
> between handlers, as opposed to an instantiation per thread (my change),
> the cost of a nextInt() invocation is minimal, and it's not a
> particularly large piece of state to track. It's commented, so there's
> no code clarity issue. So what's the problem?
>
> --Peter
>
> > -----Original Message-----
> > From: Harmeet Bedi [mailto:harmeet@kodemuse.com]
> > Sent: Monday, August 05, 2002 1:59 PM
> > To: James Developers List
> > Subject: Re: [PATCH] SMTPHandler.java v.2 - SMTP ID, Random.nextInt
> >
> > Here is what I see in the latest code for variable smtpID.
> >
> > Declaration:
> > private String smtpID;
> >
> > in <handleConnection>
> > smtpID = Math.abs(random.nextInt() % 1024) + "";
> >
> > in <resetState>
> > state.put(SMTP_ID, smtpID);
> >
> >
> > for state 'SMTP_ID'
> >
> > in <resetState>
> > state.put(SMTP_ID, smtpID);
> >
> > in <doDATA>:
> > headers.addHeaderLine(
> > "Received: from "
> > + state.get(REMOTE_NAME)
> > + " (["
> > + state.get(REMOTE_IP)
> > + "])");
> > String temp =
> > " by "
> > + this.helloName
> > + " ("
> > + softwaretype
> > + ") with SMTP ID "
> > + state.get(SMTP_ID);
> > .....
> > headers.addHeaderLine(temp);
> >
> >
> >
> > If SMTP_ID only contains random information, it really
> > contains no meaningful information. What is the gain of having it in
> the
> > mail header body ?
> >
> > How can a random integer value identify a particular thread/connection
> > ?
> >
> > The name 'SMTP_ID' lead me to believe this was the smtp server
> > identifier and that may make sense. There may be a need to identify
> > from the message header the routing mail server. I think helo name
> > parameter could be used to do this too, as you suggested. It may be a
> > good idea to rename or remove SMTP_ID and smtpID in that case.
> >
> > Harmeet
> > ----- Original Message -----
> > From: "Peter M. Goldstein" <pe...@yahoo.com>
> > To: "'James Developers List'" <ja...@jakarta.apache.org>
> > Sent: Monday, August 05, 2002 10:37 AM
> > Subject: RE: [PATCH] SMTPHandler.java v.2 - SMTP ID, Random.nextInt
> >
> >
> > >
> > > Harmeet,
> > >
> > > As written, both before and after the patch, this simply isn't what
> the
> > > SMTP ID does. The SMTP ID is written to help identify the
> particular
> > > thread/connection associated with the handler. That's all. That's
> why
> > > it's randomly drawn from a pool of ids [0,1024). It is in no way
> > > intended to identify a particular mail server.
> > >
> > > The helloName parameter can be used to address the issue you
> suggest.
> > > Each server in your cluster can be given a distinguished helloName.
> > >
> > > --Peter
> > >
> > > > -----Original Message-----
> > > > From: Harmeet Bedi [mailto:harmeet@kodemuse.com]
> > > > Sent: Monday, August 05, 2002 1:29 PM
> > > > To: James Developers List
> > > > Subject: Re: [PATCH] SMTPHandler.java v.2 - SMTP ID,
> Random.nextInt
> > > >
> > > > From: "Peter M. Goldstein"
> > > >
> > > > > In addition, the synchronized wrapper around the call to
> > > > > random.nextInt() has also been removed. After consultation with
> the
> > > > > documentation I realized that Random is completely thread-safe
> by
> > > design
> > > > > and hence no external synchronization is required.
> > > >
> > > > This is called in the context of creating and SMTP ID. I think
> SMTP ID
> > > is
> > > > needed. It may be a nice convention, but one that is not uniformly
> > > > followed.
> > > > Software version may be sufficient. In any case random smtp id,
> > > doesn't
> > > > provide new information. If we do want to put out an smtp id, it
> may
> > > be
> > > > best
> > > > to have it as a configurable setting. This may be used to identify
> on
> > > mail
> > > > server out of a cluster of mail servers.
> > > >
> > > > Harmeet
> > > > ----- Original Message -----
> > > > From: "Peter M. Goldstein" <pe...@yahoo.com>
> > > > To: "'James Developers List'" <ja...@jakarta.apache.org>
> > > > Sent: Sunday, August 04, 2002 10:26 AM
> > > > Subject: [PATCH] SMTPHandler.java v.2
> > > >
> > > >
> > > > >
> > > > > All,
> > > > >
> > > > > Attached is a patch to the SMTPHandler.java code that rolls in
> the
> > > > > String.equalsIgnoreCase change that Noel suggested.
> > > > >
> > > > > In addition, the synchronized wrapper around the call to
> > > > > random.nextInt() has also been removed. After consultation with
> the
> > > > > documentation I realized that Random is completely thread-safe
> by
> > > design
> > > > > and hence no external synchronization is required.
> > > > >
> > > > > All other previously made changes were left intact.
> > > > >
> > > > > Attached is the source and diff. As always, comments,
> criticisms,
> > > and
> > > > > questions are welcome.
> > > > >
> > > > > --Peter
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Noel J. Bergman [mailto:noel@devtech.com]
> > > > > > Sent: Saturday, August 03, 2002 8:59 PM
> > > > > > To: James-Dev Mailing List
> > > > > > Subject: String.equalsIgnoreCase Considered Evil :-)
> > > > > >
> > > > > > There are many places in James where we have lengthy sets of
> > > > > comparisons
> > > > > > of
> > > > > > an unknown string value to a known string value. These are
> many
> > > of
> > > > > these
> > > > > > of
> > > > > > the form:
> > > > > >
> > > > > > string.equalsIgnoreCase(<literal>)
> > > > > >
> > > > > > This use is terribly inefficient. Each and every call
> iterates
> > > > > through
> > > > > > both
> > > > > > the string and the literal, uppercasing both as it compares
> them.
> > > > > >
> > > > > > Please, as you are working on code (Peter will apply these to
> > > > > POP3Handler
> > > > > > and SMTPHandler), if you see this pattern, please change it
> to:
> > > > > >
> > > > > > string = string.to[Upper|Lower]Case(); // chose depending
> upon
> > > > > your
> > > > > > literals
> > > > > >
> > > > > > string.equals(<literal>)
> > > > > >
> > > > > > For example, in SMTPHandler and POP3Handler:
> > > > > >
> > > > > > String command = commandRaw.trim();
> > > > > > becomes
> > > > > > String command = commandRaw.trim().toUpperCase();
> > > > > >
> > > > > > and the test for "USER" (for example) becomes:
> > > > > >
> > > > > > if (command.equals("USER")) ...
> > > > > >
> > > > > > Actually, I believe that we should add a command-map model to
> the
> > > > > > handlers,
> > > > > > but that's a seperate issue for a separate thread. The change
> > > > > proposed in
> > > > > > this e-mail is simple.
> > > > > >
> > > > > > --- Noel
> > > > > >
> > > > > > P.S. Brownie points for whomever recognizes the origin of the
> > > subject
> > > > > > header
> > > > > >
> > > > > >
> > > > > > --
> > > > > > To unsubscribe, e-mail: <mailto:james-dev-
> > > > > > unsubscribe@jakarta.apache.org>
> > > > > > For additional commands, e-mail: <mailto:james-dev-
> > > > > > help@jakarta.apache.org>
> > > > >
> > > >
> > > >
> > > >
> > >
> ------------------------------------------------------------------------
> > > --
> > > > --
> > > > ----
> > > >
> > > >
> > > > > --
> > > > > To unsubscribe, e-mail:
> > > > <ma...@jakarta.apache.org>
> > > > > For additional commands, e-mail:
> > > > <ma...@jakarta.apache.org>
> > > >
> > > >
> > > > --
> > > > To unsubscribe, e-mail: <mailto:james-dev-
> > > > unsubscribe@jakarta.apache.org>
> > > > For additional commands, e-mail: <mailto:james-dev-
> > > > help@jakarta.apache.org>
> > >
> > >
> > >
> > > --
> > > To unsubscribe, e-mail:
> > <ma...@jakarta.apache.org>
> > > For additional commands, e-mail:
> > <ma...@jakarta.apache.org>
> >
> >
> > --
> > To unsubscribe, e-mail: <mailto:james-dev-
> > unsubscribe@jakarta.apache.org>
> > For additional commands, e-mail: <mailto:james-dev-
> > help@jakarta.apache.org>
>
>
>
> --
> To unsubscribe, e-mail:
<ma...@jakarta.apache.org>
> For additional commands, e-mail:
<ma...@jakarta.apache.org>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: [PATCH] SMTPHandler.java v.2 - SMTP ID, Random.nextInt
Posted by "Peter M. Goldstein" <pe...@yahoo.com>.
Harmeet,
I just don't get your objection.
SMTP ID does one thing and one thing only - allows you to identify from
the external SMTP exchange which connection/thread it was associated
with. It's not a tremendously useful feature I grant you, but in the
case of an extreme fatal error (one that resulted in a thread dump) it
may very well allow you to connection the external SMTP connection with
the single thread among potentially hundreds that was connecting to a
particular SMTP exchange. Presumably it was added to the code base for
precisely that reason. I'm reluctant to change an exposed, functioning
piece of code from the software without any sort of compelling reason.
The cost of this feature is negligible. The Random is now shared
between handlers, as opposed to an instantiation per thread (my change),
the cost of a nextInt() invocation is minimal, and it's not a
particularly large piece of state to track. It's commented, so there's
no code clarity issue. So what's the problem?
--Peter
> -----Original Message-----
> From: Harmeet Bedi [mailto:harmeet@kodemuse.com]
> Sent: Monday, August 05, 2002 1:59 PM
> To: James Developers List
> Subject: Re: [PATCH] SMTPHandler.java v.2 - SMTP ID, Random.nextInt
>
> Here is what I see in the latest code for variable smtpID.
>
> Declaration:
> private String smtpID;
>
> in <handleConnection>
> smtpID = Math.abs(random.nextInt() % 1024) + "";
>
> in <resetState>
> state.put(SMTP_ID, smtpID);
>
>
> for state 'SMTP_ID'
>
> in <resetState>
> state.put(SMTP_ID, smtpID);
>
> in <doDATA>:
> headers.addHeaderLine(
> "Received: from "
> + state.get(REMOTE_NAME)
> + " (["
> + state.get(REMOTE_IP)
> + "])");
> String temp =
> " by "
> + this.helloName
> + " ("
> + softwaretype
> + ") with SMTP ID "
> + state.get(SMTP_ID);
> .....
> headers.addHeaderLine(temp);
>
>
>
> If SMTP_ID only contains random information, it really
> contains no meaningful information. What is the gain of having it in
the
> mail header body ?
>
> How can a random integer value identify a particular thread/connection
> ?
>
> The name 'SMTP_ID' lead me to believe this was the smtp server
> identifier and that may make sense. There may be a need to identify
> from the message header the routing mail server. I think helo name
> parameter could be used to do this too, as you suggested. It may be a
> good idea to rename or remove SMTP_ID and smtpID in that case.
>
> Harmeet
> ----- Original Message -----
> From: "Peter M. Goldstein" <pe...@yahoo.com>
> To: "'James Developers List'" <ja...@jakarta.apache.org>
> Sent: Monday, August 05, 2002 10:37 AM
> Subject: RE: [PATCH] SMTPHandler.java v.2 - SMTP ID, Random.nextInt
>
>
> >
> > Harmeet,
> >
> > As written, both before and after the patch, this simply isn't what
the
> > SMTP ID does. The SMTP ID is written to help identify the
particular
> > thread/connection associated with the handler. That's all. That's
why
> > it's randomly drawn from a pool of ids [0,1024). It is in no way
> > intended to identify a particular mail server.
> >
> > The helloName parameter can be used to address the issue you
suggest.
> > Each server in your cluster can be given a distinguished helloName.
> >
> > --Peter
> >
> > > -----Original Message-----
> > > From: Harmeet Bedi [mailto:harmeet@kodemuse.com]
> > > Sent: Monday, August 05, 2002 1:29 PM
> > > To: James Developers List
> > > Subject: Re: [PATCH] SMTPHandler.java v.2 - SMTP ID,
Random.nextInt
> > >
> > > From: "Peter M. Goldstein"
> > >
> > > > In addition, the synchronized wrapper around the call to
> > > > random.nextInt() has also been removed. After consultation with
the
> > > > documentation I realized that Random is completely thread-safe
by
> > design
> > > > and hence no external synchronization is required.
> > >
> > > This is called in the context of creating and SMTP ID. I think
SMTP ID
> > is
> > > needed. It may be a nice convention, but one that is not uniformly
> > > followed.
> > > Software version may be sufficient. In any case random smtp id,
> > doesn't
> > > provide new information. If we do want to put out an smtp id, it
may
> > be
> > > best
> > > to have it as a configurable setting. This may be used to identify
on
> > mail
> > > server out of a cluster of mail servers.
> > >
> > > Harmeet
> > > ----- Original Message -----
> > > From: "Peter M. Goldstein" <pe...@yahoo.com>
> > > To: "'James Developers List'" <ja...@jakarta.apache.org>
> > > Sent: Sunday, August 04, 2002 10:26 AM
> > > Subject: [PATCH] SMTPHandler.java v.2
> > >
> > >
> > > >
> > > > All,
> > > >
> > > > Attached is a patch to the SMTPHandler.java code that rolls in
the
> > > > String.equalsIgnoreCase change that Noel suggested.
> > > >
> > > > In addition, the synchronized wrapper around the call to
> > > > random.nextInt() has also been removed. After consultation with
the
> > > > documentation I realized that Random is completely thread-safe
by
> > design
> > > > and hence no external synchronization is required.
> > > >
> > > > All other previously made changes were left intact.
> > > >
> > > > Attached is the source and diff. As always, comments,
criticisms,
> > and
> > > > questions are welcome.
> > > >
> > > > --Peter
> > > >
> > > > > -----Original Message-----
> > > > > From: Noel J. Bergman [mailto:noel@devtech.com]
> > > > > Sent: Saturday, August 03, 2002 8:59 PM
> > > > > To: James-Dev Mailing List
> > > > > Subject: String.equalsIgnoreCase Considered Evil :-)
> > > > >
> > > > > There are many places in James where we have lengthy sets of
> > > > comparisons
> > > > > of
> > > > > an unknown string value to a known string value. These are
many
> > of
> > > > these
> > > > > of
> > > > > the form:
> > > > >
> > > > > string.equalsIgnoreCase(<literal>)
> > > > >
> > > > > This use is terribly inefficient. Each and every call
iterates
> > > > through
> > > > > both
> > > > > the string and the literal, uppercasing both as it compares
them.
> > > > >
> > > > > Please, as you are working on code (Peter will apply these to
> > > > POP3Handler
> > > > > and SMTPHandler), if you see this pattern, please change it
to:
> > > > >
> > > > > string = string.to[Upper|Lower]Case(); // chose depending
upon
> > > > your
> > > > > literals
> > > > >
> > > > > string.equals(<literal>)
> > > > >
> > > > > For example, in SMTPHandler and POP3Handler:
> > > > >
> > > > > String command = commandRaw.trim();
> > > > > becomes
> > > > > String command = commandRaw.trim().toUpperCase();
> > > > >
> > > > > and the test for "USER" (for example) becomes:
> > > > >
> > > > > if (command.equals("USER")) ...
> > > > >
> > > > > Actually, I believe that we should add a command-map model to
the
> > > > > handlers,
> > > > > but that's a seperate issue for a separate thread. The change
> > > > proposed in
> > > > > this e-mail is simple.
> > > > >
> > > > > --- Noel
> > > > >
> > > > > P.S. Brownie points for whomever recognizes the origin of the
> > subject
> > > > > header
> > > > >
> > > > >
> > > > > --
> > > > > To unsubscribe, e-mail: <mailto:james-dev-
> > > > > unsubscribe@jakarta.apache.org>
> > > > > For additional commands, e-mail: <mailto:james-dev-
> > > > > help@jakarta.apache.org>
> > > >
> > >
> > >
> > >
> >
------------------------------------------------------------------------
> > --
> > > --
> > > ----
> > >
> > >
> > > > --
> > > > To unsubscribe, e-mail:
> > > <ma...@jakarta.apache.org>
> > > > For additional commands, e-mail:
> > > <ma...@jakarta.apache.org>
> > >
> > >
> > > --
> > > To unsubscribe, e-mail: <mailto:james-dev-
> > > unsubscribe@jakarta.apache.org>
> > > For additional commands, e-mail: <mailto:james-dev-
> > > help@jakarta.apache.org>
> >
> >
> >
> > --
> > To unsubscribe, e-mail:
> <ma...@jakarta.apache.org>
> > For additional commands, e-mail:
> <ma...@jakarta.apache.org>
>
>
> --
> To unsubscribe, e-mail: <mailto:james-dev-
> unsubscribe@jakarta.apache.org>
> For additional commands, e-mail: <mailto:james-dev-
> help@jakarta.apache.org>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] SMTPHandler.java v.2 - SMTP ID, Random.nextInt
Posted by Harmeet Bedi <ha...@kodemuse.com>.
Here is what I see in the latest code for variable smtpID.
Declaration:
private String smtpID;
in <handleConnection>
smtpID = Math.abs(random.nextInt() % 1024) + "";
in <resetState>
state.put(SMTP_ID, smtpID);
for state 'SMTP_ID'
in <resetState>
state.put(SMTP_ID, smtpID);
in <doDATA>:
headers.addHeaderLine(
"Received: from "
+ state.get(REMOTE_NAME)
+ " (["
+ state.get(REMOTE_IP)
+ "])");
String temp =
" by "
+ this.helloName
+ " ("
+ softwaretype
+ ") with SMTP ID "
+ state.get(SMTP_ID);
.....
headers.addHeaderLine(temp);
If SMTP_ID only contains random information, it really
contains no meaningful information. What is the gain of having it in the
mail header body ?
How can a random integer value identify a particular thread/connection
?
The name 'SMTP_ID' lead me to believe this was the smtp server
identifier and that may make sense. There may be a need to identify
from the message header the routing mail server. I think helo name
parameter could be used to do this too, as you suggested. It may be a
good idea to rename or remove SMTP_ID and smtpID in that case.
Harmeet
----- Original Message -----
From: "Peter M. Goldstein" <pe...@yahoo.com>
To: "'James Developers List'" <ja...@jakarta.apache.org>
Sent: Monday, August 05, 2002 10:37 AM
Subject: RE: [PATCH] SMTPHandler.java v.2 - SMTP ID, Random.nextInt
>
> Harmeet,
>
> As written, both before and after the patch, this simply isn't what the
> SMTP ID does. The SMTP ID is written to help identify the particular
> thread/connection associated with the handler. That's all. That's why
> it's randomly drawn from a pool of ids [0,1024). It is in no way
> intended to identify a particular mail server.
>
> The helloName parameter can be used to address the issue you suggest.
> Each server in your cluster can be given a distinguished helloName.
>
> --Peter
>
> > -----Original Message-----
> > From: Harmeet Bedi [mailto:harmeet@kodemuse.com]
> > Sent: Monday, August 05, 2002 1:29 PM
> > To: James Developers List
> > Subject: Re: [PATCH] SMTPHandler.java v.2 - SMTP ID, Random.nextInt
> >
> > From: "Peter M. Goldstein"
> >
> > > In addition, the synchronized wrapper around the call to
> > > random.nextInt() has also been removed. After consultation with the
> > > documentation I realized that Random is completely thread-safe by
> design
> > > and hence no external synchronization is required.
> >
> > This is called in the context of creating and SMTP ID. I think SMTP ID
> is
> > needed. It may be a nice convention, but one that is not uniformly
> > followed.
> > Software version may be sufficient. In any case random smtp id,
> doesn't
> > provide new information. If we do want to put out an smtp id, it may
> be
> > best
> > to have it as a configurable setting. This may be used to identify on
> mail
> > server out of a cluster of mail servers.
> >
> > Harmeet
> > ----- Original Message -----
> > From: "Peter M. Goldstein" <pe...@yahoo.com>
> > To: "'James Developers List'" <ja...@jakarta.apache.org>
> > Sent: Sunday, August 04, 2002 10:26 AM
> > Subject: [PATCH] SMTPHandler.java v.2
> >
> >
> > >
> > > All,
> > >
> > > Attached is a patch to the SMTPHandler.java code that rolls in the
> > > String.equalsIgnoreCase change that Noel suggested.
> > >
> > > In addition, the synchronized wrapper around the call to
> > > random.nextInt() has also been removed. After consultation with the
> > > documentation I realized that Random is completely thread-safe by
> design
> > > and hence no external synchronization is required.
> > >
> > > All other previously made changes were left intact.
> > >
> > > Attached is the source and diff. As always, comments, criticisms,
> and
> > > questions are welcome.
> > >
> > > --Peter
> > >
> > > > -----Original Message-----
> > > > From: Noel J. Bergman [mailto:noel@devtech.com]
> > > > Sent: Saturday, August 03, 2002 8:59 PM
> > > > To: James-Dev Mailing List
> > > > Subject: String.equalsIgnoreCase Considered Evil :-)
> > > >
> > > > There are many places in James where we have lengthy sets of
> > > comparisons
> > > > of
> > > > an unknown string value to a known string value. These are many
> of
> > > these
> > > > of
> > > > the form:
> > > >
> > > > string.equalsIgnoreCase(<literal>)
> > > >
> > > > This use is terribly inefficient. Each and every call iterates
> > > through
> > > > both
> > > > the string and the literal, uppercasing both as it compares them.
> > > >
> > > > Please, as you are working on code (Peter will apply these to
> > > POP3Handler
> > > > and SMTPHandler), if you see this pattern, please change it to:
> > > >
> > > > string = string.to[Upper|Lower]Case(); // chose depending upon
> > > your
> > > > literals
> > > >
> > > > string.equals(<literal>)
> > > >
> > > > For example, in SMTPHandler and POP3Handler:
> > > >
> > > > String command = commandRaw.trim();
> > > > becomes
> > > > String command = commandRaw.trim().toUpperCase();
> > > >
> > > > and the test for "USER" (for example) becomes:
> > > >
> > > > if (command.equals("USER")) ...
> > > >
> > > > Actually, I believe that we should add a command-map model to the
> > > > handlers,
> > > > but that's a seperate issue for a separate thread. The change
> > > proposed in
> > > > this e-mail is simple.
> > > >
> > > > --- Noel
> > > >
> > > > P.S. Brownie points for whomever recognizes the origin of the
> subject
> > > > header
> > > >
> > > >
> > > > --
> > > > To unsubscribe, e-mail: <mailto:james-dev-
> > > > unsubscribe@jakarta.apache.org>
> > > > For additional commands, e-mail: <mailto:james-dev-
> > > > help@jakarta.apache.org>
> > >
> >
> >
> >
> ------------------------------------------------------------------------
> --
> > --
> > ----
> >
> >
> > > --
> > > To unsubscribe, e-mail:
> > <ma...@jakarta.apache.org>
> > > For additional commands, e-mail:
> > <ma...@jakarta.apache.org>
> >
> >
> > --
> > To unsubscribe, e-mail: <mailto:james-dev-
> > unsubscribe@jakarta.apache.org>
> > For additional commands, e-mail: <mailto:james-dev-
> > help@jakarta.apache.org>
>
>
>
> --
> To unsubscribe, e-mail:
<ma...@jakarta.apache.org>
> For additional commands, e-mail:
<ma...@jakarta.apache.org>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: [PATCH] SMTPHandler.java v.2 - SMTP ID, Random.nextInt
Posted by "Peter M. Goldstein" <pe...@yahoo.com>.
Harmeet,
As written, both before and after the patch, this simply isn't what the
SMTP ID does. The SMTP ID is written to help identify the particular
thread/connection associated with the handler. That's all. That's why
it's randomly drawn from a pool of ids [0,1024). It is in no way
intended to identify a particular mail server.
The helloName parameter can be used to address the issue you suggest.
Each server in your cluster can be given a distinguished helloName.
--Peter
> -----Original Message-----
> From: Harmeet Bedi [mailto:harmeet@kodemuse.com]
> Sent: Monday, August 05, 2002 1:29 PM
> To: James Developers List
> Subject: Re: [PATCH] SMTPHandler.java v.2 - SMTP ID, Random.nextInt
>
> From: "Peter M. Goldstein"
>
> > In addition, the synchronized wrapper around the call to
> > random.nextInt() has also been removed. After consultation with the
> > documentation I realized that Random is completely thread-safe by
design
> > and hence no external synchronization is required.
>
> This is called in the context of creating and SMTP ID. I think SMTP ID
is
> needed. It may be a nice convention, but one that is not uniformly
> followed.
> Software version may be sufficient. In any case random smtp id,
doesn't
> provide new information. If we do want to put out an smtp id, it may
be
> best
> to have it as a configurable setting. This may be used to identify on
mail
> server out of a cluster of mail servers.
>
> Harmeet
> ----- Original Message -----
> From: "Peter M. Goldstein" <pe...@yahoo.com>
> To: "'James Developers List'" <ja...@jakarta.apache.org>
> Sent: Sunday, August 04, 2002 10:26 AM
> Subject: [PATCH] SMTPHandler.java v.2
>
>
> >
> > All,
> >
> > Attached is a patch to the SMTPHandler.java code that rolls in the
> > String.equalsIgnoreCase change that Noel suggested.
> >
> > In addition, the synchronized wrapper around the call to
> > random.nextInt() has also been removed. After consultation with the
> > documentation I realized that Random is completely thread-safe by
design
> > and hence no external synchronization is required.
> >
> > All other previously made changes were left intact.
> >
> > Attached is the source and diff. As always, comments, criticisms,
and
> > questions are welcome.
> >
> > --Peter
> >
> > > -----Original Message-----
> > > From: Noel J. Bergman [mailto:noel@devtech.com]
> > > Sent: Saturday, August 03, 2002 8:59 PM
> > > To: James-Dev Mailing List
> > > Subject: String.equalsIgnoreCase Considered Evil :-)
> > >
> > > There are many places in James where we have lengthy sets of
> > comparisons
> > > of
> > > an unknown string value to a known string value. These are many
of
> > these
> > > of
> > > the form:
> > >
> > > string.equalsIgnoreCase(<literal>)
> > >
> > > This use is terribly inefficient. Each and every call iterates
> > through
> > > both
> > > the string and the literal, uppercasing both as it compares them.
> > >
> > > Please, as you are working on code (Peter will apply these to
> > POP3Handler
> > > and SMTPHandler), if you see this pattern, please change it to:
> > >
> > > string = string.to[Upper|Lower]Case(); // chose depending upon
> > your
> > > literals
> > >
> > > string.equals(<literal>)
> > >
> > > For example, in SMTPHandler and POP3Handler:
> > >
> > > String command = commandRaw.trim();
> > > becomes
> > > String command = commandRaw.trim().toUpperCase();
> > >
> > > and the test for "USER" (for example) becomes:
> > >
> > > if (command.equals("USER")) ...
> > >
> > > Actually, I believe that we should add a command-map model to the
> > > handlers,
> > > but that's a seperate issue for a separate thread. The change
> > proposed in
> > > this e-mail is simple.
> > >
> > > --- Noel
> > >
> > > P.S. Brownie points for whomever recognizes the origin of the
subject
> > > header
> > >
> > >
> > > --
> > > To unsubscribe, e-mail: <mailto:james-dev-
> > > unsubscribe@jakarta.apache.org>
> > > For additional commands, e-mail: <mailto:james-dev-
> > > help@jakarta.apache.org>
> >
>
>
>
------------------------------------------------------------------------
--
> --
> ----
>
>
> > --
> > To unsubscribe, e-mail:
> <ma...@jakarta.apache.org>
> > For additional commands, e-mail:
> <ma...@jakarta.apache.org>
>
>
> --
> To unsubscribe, e-mail: <mailto:james-dev-
> unsubscribe@jakarta.apache.org>
> For additional commands, e-mail: <mailto:james-dev-
> help@jakarta.apache.org>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] SMTPHandler.java v.2 - SMTP ID, Random.nextInt
Posted by Harmeet Bedi <ha...@kodemuse.com>.
From: "Peter M. Goldstein"
> In addition, the synchronized wrapper around the call to
> random.nextInt() has also been removed. After consultation with the
> documentation I realized that Random is completely thread-safe by design
> and hence no external synchronization is required.
This is called in the context of creating and SMTP ID. I think SMTP ID is
needed. It may be a nice convention, but one that is not uniformly followed.
Software version may be sufficient. In any case random smtp id, doesn't
provide new information. If we do want to put out an smtp id, it may be best
to have it as a configurable setting. This may be used to identify on mail
server out of a cluster of mail servers.
Harmeet
----- Original Message -----
From: "Peter M. Goldstein" <pe...@yahoo.com>
To: "'James Developers List'" <ja...@jakarta.apache.org>
Sent: Sunday, August 04, 2002 10:26 AM
Subject: [PATCH] SMTPHandler.java v.2
>
> All,
>
> Attached is a patch to the SMTPHandler.java code that rolls in the
> String.equalsIgnoreCase change that Noel suggested.
>
> In addition, the synchronized wrapper around the call to
> random.nextInt() has also been removed. After consultation with the
> documentation I realized that Random is completely thread-safe by design
> and hence no external synchronization is required.
>
> All other previously made changes were left intact.
>
> Attached is the source and diff. As always, comments, criticisms, and
> questions are welcome.
>
> --Peter
>
> > -----Original Message-----
> > From: Noel J. Bergman [mailto:noel@devtech.com]
> > Sent: Saturday, August 03, 2002 8:59 PM
> > To: James-Dev Mailing List
> > Subject: String.equalsIgnoreCase Considered Evil :-)
> >
> > There are many places in James where we have lengthy sets of
> comparisons
> > of
> > an unknown string value to a known string value. These are many of
> these
> > of
> > the form:
> >
> > string.equalsIgnoreCase(<literal>)
> >
> > This use is terribly inefficient. Each and every call iterates
> through
> > both
> > the string and the literal, uppercasing both as it compares them.
> >
> > Please, as you are working on code (Peter will apply these to
> POP3Handler
> > and SMTPHandler), if you see this pattern, please change it to:
> >
> > string = string.to[Upper|Lower]Case(); // chose depending upon
> your
> > literals
> >
> > string.equals(<literal>)
> >
> > For example, in SMTPHandler and POP3Handler:
> >
> > String command = commandRaw.trim();
> > becomes
> > String command = commandRaw.trim().toUpperCase();
> >
> > and the test for "USER" (for example) becomes:
> >
> > if (command.equals("USER")) ...
> >
> > Actually, I believe that we should add a command-map model to the
> > handlers,
> > but that's a seperate issue for a separate thread. The change
> proposed in
> > this e-mail is simple.
> >
> > --- Noel
> >
> > P.S. Brownie points for whomever recognizes the origin of the subject
> > header
> >
> >
> > --
> > To unsubscribe, e-mail: <mailto:james-dev-
> > unsubscribe@jakarta.apache.org>
> > For additional commands, e-mail: <mailto:james-dev-
> > help@jakarta.apache.org>
>
----------------------------------------------------------------------------
----
> --
> To unsubscribe, e-mail:
<ma...@jakarta.apache.org>
> For additional commands, e-mail:
<ma...@jakarta.apache.org>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: [PATCH] SMTPHandler.java v.2
Posted by "Peter M. Goldstein" <pe...@yahoo.com>.
Hontvari,
> + public final static String NAME_GIVEN = "NAME_GIVEN"; //
Remote
> host name provided by
> + //
client
>
> You should follow the code conventions when commenting member
variables,
> otherwise they won't show up on javadocs, e.g. /** classVar1
documentation
> comment */
Actually I don't really want these comments to show up on javadocs.
I think the better solution is to make these fields private. They are
exclusively for use in the SMTPHandler, so they actually shouldn't be
public. Comments associated with private variables do not get displayed
in javadoc.
>
> + out.flush();
> + logResponseString(responseString);
>
> sometimes flushing and logging are in the opposite order in the patch.
Agreed, it should be uniform. I've fixed that.
--Peter
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] SMTPHandler.java v.2
Posted by Hontvari Jozsef <ho...@solware.com>.
+ public final static String NAME_GIVEN = "NAME_GIVEN"; // Remote
host name provided by
+ // client
You should follow the code conventions when commenting member variables,
otherwise they won't show up on javadocs, e.g. /** classVar1 documentation
comment */
+ out.flush();
+ logResponseString(responseString);
sometimes flushing and logging are in the opposite order in the patch.
I had no time to check the second half of the patch. Thanks for your work.
----- Original Message -----
From: "Peter M. Goldstein" <pe...@yahoo.com>
To: "'James Developers List'" <ja...@jakarta.apache.org>
Sent: Sunday, August 04, 2002 7:26 PM
Subject: [PATCH] SMTPHandler.java v.2
>
> All,
>
> Attached is a patch to the SMTPHandler.java code that rolls in the
> String.equalsIgnoreCase change that Noel suggested.
>
> In addition, the synchronized wrapper around the call to
> random.nextInt() has also been removed. After consultation with the
> documentation I realized that Random is completely thread-safe by design
> and hence no external synchronization is required.
>
> All other previously made changes were left intact.
>
> Attached is the source and diff. As always, comments, criticisms, and
> questions are welcome.
>
> --Peter
>
> > -----Original Message-----
> > From: Noel J. Bergman [mailto:noel@devtech.com]
> > Sent: Saturday, August 03, 2002 8:59 PM
> > To: James-Dev Mailing List
> > Subject: String.equalsIgnoreCase Considered Evil :-)
> >
> > There are many places in James where we have lengthy sets of
> comparisons
> > of
> > an unknown string value to a known string value. These are many of
> these
> > of
> > the form:
> >
> > string.equalsIgnoreCase(<literal>)
> >
> > This use is terribly inefficient. Each and every call iterates
> through
> > both
> > the string and the literal, uppercasing both as it compares them.
> >
> > Please, as you are working on code (Peter will apply these to
> POP3Handler
> > and SMTPHandler), if you see this pattern, please change it to:
> >
> > string = string.to[Upper|Lower]Case(); // chose depending upon
> your
> > literals
> >
> > string.equals(<literal>)
> >
> > For example, in SMTPHandler and POP3Handler:
> >
> > String command = commandRaw.trim();
> > becomes
> > String command = commandRaw.trim().toUpperCase();
> >
> > and the test for "USER" (for example) becomes:
> >
> > if (command.equals("USER")) ...
> >
> > Actually, I believe that we should add a command-map model to the
> > handlers,
> > but that's a seperate issue for a separate thread. The change
> proposed in
> > this e-mail is simple.
> >
> > --- Noel
> >
> > P.S. Brownie points for whomever recognizes the origin of the subject
> > header
> >
> >
> > --
> > To unsubscribe, e-mail: <mailto:james-dev-
> > unsubscribe@jakarta.apache.org>
> > For additional commands, e-mail: <mailto:james-dev-
> > help@jakarta.apache.org>
>
----------------------------------------------------------------------------
----
> --
> To unsubscribe, e-mail:
<ma...@jakarta.apache.org>
> For additional commands, e-mail:
<ma...@jakarta.apache.org>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
[PATCH] SMTPHandler.java v.2
Posted by "Peter M. Goldstein" <pe...@yahoo.com>.
All,
Attached is a patch to the SMTPHandler.java code that rolls in the
String.equalsIgnoreCase change that Noel suggested.
In addition, the synchronized wrapper around the call to
random.nextInt() has also been removed. After consultation with the
documentation I realized that Random is completely thread-safe by design
and hence no external synchronization is required.
All other previously made changes were left intact.
Attached is the source and diff. As always, comments, criticisms, and
questions are welcome.
--Peter
> -----Original Message-----
> From: Noel J. Bergman [mailto:noel@devtech.com]
> Sent: Saturday, August 03, 2002 8:59 PM
> To: James-Dev Mailing List
> Subject: String.equalsIgnoreCase Considered Evil :-)
>
> There are many places in James where we have lengthy sets of
comparisons
> of
> an unknown string value to a known string value. These are many of
these
> of
> the form:
>
> string.equalsIgnoreCase(<literal>)
>
> This use is terribly inefficient. Each and every call iterates
through
> both
> the string and the literal, uppercasing both as it compares them.
>
> Please, as you are working on code (Peter will apply these to
POP3Handler
> and SMTPHandler), if you see this pattern, please change it to:
>
> string = string.to[Upper|Lower]Case(); // chose depending upon
your
> literals
>
> string.equals(<literal>)
>
> For example, in SMTPHandler and POP3Handler:
>
> String command = commandRaw.trim();
> becomes
> String command = commandRaw.trim().toUpperCase();
>
> and the test for "USER" (for example) becomes:
>
> if (command.equals("USER")) ...
>
> Actually, I believe that we should add a command-map model to the
> handlers,
> but that's a seperate issue for a separate thread. The change
proposed in
> this e-mail is simple.
>
> --- Noel
>
> P.S. Brownie points for whomever recognizes the origin of the subject
> header
>
>
> --
> To unsubscribe, e-mail: <mailto:james-dev-
> unsubscribe@jakarta.apache.org>
> For additional commands, e-mail: <mailto:james-dev-
> help@jakarta.apache.org>