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 "Peter M. Goldstein" <pe...@yahoo.com> on 2002/08/04 19:39:43 UTC
[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>
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>