You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Gordon Sim <gs...@redhat.com> on 2008/07/01 19:30:35 UTC
[java]: commented out code - can it be deleted?
In the notifyMessage(UnprocessedMessage):void method of
BasicMessageConsumer there are still some commented out lines from a
very old commit (merged to trunk over a year ago).
Am I right in assuming there is no real reason for this being commented
out and that it can therefore be deleted?
I would just delete it quietly, but as I don't have the context for the
change that required disabling these lines of code, I thought a quick
mail might be in order (in case they were serving as some sort of
reminder or something?!).
I'm not picking on java, honest! I'm sure there are similar things in
e.g. the c++ code. I'm pointing this case out because it just came up in
conversation. I'd be as happy for people to point out issues they notice
on any code I tend to work on; once noticed its easy to clean up.
The biggest chunk of code in question looks like:
// synchronized (_closed)
{
// if (!_closed.get())
{
//preDeliver(jmsMessage);
notifyMessage(jmsMessage);
}
// else
// {
// _logger.error("MESSAGE REJECTING!");
// _session.rejectMessage(jmsMessage, true);
// //_logger.error("MESSAGE JUST DROPPED!");
// }
but the '// synchronized (_closed)' part is replicated in several places.
Re: [java]: commented out code - can it be deleted?
Posted by Alan Conway <ac...@redhat.com>.
On Wed, 2008-07-02 at 09:44 +0100, Martin Ritchie wrote:
> 2008/7/1 Gordon Sim <gs...@redhat.com>:
> > In the notifyMessage(UnprocessedMessage):void method of BasicMessageConsumer
> > there are still some commented out lines from a very old commit (merged to
> > trunk over a year ago).
> >
> > Am I right in assuming there is no real reason for this being commented out
> > and that it can therefore be deleted?
> >
> > I would just delete it quietly, but as I don't have the context for the
> > change that required disabling these lines of code, I thought a quick mail
> > might be in order (in case they were serving as some sort of reminder or
> > something?!).
> >
> > I'm not picking on java, honest! I'm sure there are similar things in e.g.
> > the c++ code. I'm pointing this case out because it just came up in
> > conversation. I'd be as happy for people to point out issues they notice on
> > any code I tend to work on; once noticed its easy to clean up.
> >
> > The biggest chunk of code in question looks like:
> >
> > // synchronized (_closed)
> > {
> > // if (!_closed.get())
> > {
> >
> > //preDeliver(jmsMessage);
> >
> > notifyMessage(jmsMessage);
> > }
> > // else
> > // {
> > // _logger.error("MESSAGE REJECTING!");
> > // _session.rejectMessage(jmsMessage, true);
> > // //_logger.error("MESSAGE JUST DROPPED!");
> > // }
> >
> > but the '// synchronized (_closed)' part is replicated in several places.
>
> I'm happy with the removal. IIRC the synchronization bits were just
> commented out as the client is in such a poor state wrt
> threading/locking that we were not 100% sure of the changes.
>
> However, there is no reason to be checking in commented out code (even
> if it is commented) it is just too fragile and for me just says the is
> doubt about the correctness of the change.
>
> My recent commit to the performance change is a prime example. I don't
> like the commenting out of the code but we need to decide what we are
> going to do with the broken code.. spend the time on it to fix it and
> use it or just leave it. I think just now it will be left as no one
> has time to work on it .. hence the comment and the commenting out of
> code. I was very surprised not to get a good flaming for it though.
>
> Cheers
>
> Martin
Re: [java]: commented out code - can it be deleted?
Posted by Martin Ritchie <ri...@apache.org>.
2008/7/1 Gordon Sim <gs...@redhat.com>:
> In the notifyMessage(UnprocessedMessage):void method of BasicMessageConsumer
> there are still some commented out lines from a very old commit (merged to
> trunk over a year ago).
>
> Am I right in assuming there is no real reason for this being commented out
> and that it can therefore be deleted?
>
> I would just delete it quietly, but as I don't have the context for the
> change that required disabling these lines of code, I thought a quick mail
> might be in order (in case they were serving as some sort of reminder or
> something?!).
>
> I'm not picking on java, honest! I'm sure there are similar things in e.g.
> the c++ code. I'm pointing this case out because it just came up in
> conversation. I'd be as happy for people to point out issues they notice on
> any code I tend to work on; once noticed its easy to clean up.
>
> The biggest chunk of code in question looks like:
>
> // synchronized (_closed)
> {
> // if (!_closed.get())
> {
>
> //preDeliver(jmsMessage);
>
> notifyMessage(jmsMessage);
> }
> // else
> // {
> // _logger.error("MESSAGE REJECTING!");
> // _session.rejectMessage(jmsMessage, true);
> // //_logger.error("MESSAGE JUST DROPPED!");
> // }
>
> but the '// synchronized (_closed)' part is replicated in several places.
I'm happy with the removal. IIRC the synchronization bits were just
commented out as the client is in such a poor state wrt
threading/locking that we were not 100% sure of the changes.
However, there is no reason to be checking in commented out code (even
if it is commented) it is just too fragile and for me just says the is
doubt about the correctness of the change.
My recent commit to the performance change is a prime example. I don't
like the commenting out of the code but we need to decide what we are
going to do with the broken code.. spend the time on it to fix it and
use it or just leave it. I think just now it will be left as no one
has time to work on it .. hence the comment and the commenting out of
code. I was very surprised not to get a good flaming for it though.
Cheers
Martin
--
Martin Ritchie