Re: [all] FindBugs' Switch statement found in class.method where default case is missing
Seems to me there are several reasons why the default case may have
been omitted:
1) It was accidentally omitted.
In this case, add the required clause.
2) It was omitted because nothing needs to be done.
In this case, this needs to be documented; the easiest way is to add:
default:
break; // nothing needs to be done here
3) It was omitted because it cannot happen
In this case, add an assertion or throw an error.
This is so that a future change which invalidates the assumption is
not overlooked, but is caught.
It's important that the person reading the code knows that the switch
block is complete.
So fixing the problem by changing CheckStyle or Findbugs configuration
files is not sufficient; there must be at least a comment in the code.
On 5 September 2012 01:04, Liviu Tudor liviu.tu...@gmail.com wrote:
Hi guys,
I am looking at this from a different perspective: the same check can be
performed using checkstyle
(http://checkstyle.sourceforge.net/config_coding.html#MissingSwitchDefault)
as well as FindBugs. So if this is a valid case where there is no need
for a default branch, we could perhaps simply use a configured checkstyle
comment to instruct checkstyle that this check doesn't need to be
performed in this case?
If this definitely needs FindBugs, I know that FindBugs supports
annotations, so it might be worth looking into that, though I don't know
off the top of my head if there is one available for default missing in a
switch.
Liv
Liviu Tudor
E: liviu.tu...@gmail.com
C: +1 650-2833815 (USA)
M: +44 (0)7591540313 (UK, Europe)
W: http://about.me/liviutudor
Skype: liviutudor
I'm nobody, nobody's perfect -- therefore I'm perfect!
On 04/09/2012 16:02, Gilles Sadowski gil...@harfang.homelinux.org
wrote:
FindBugs can give warnings like:
Switch statement found in
org.apache.commons.codec.binary.Base32.decode(byte[], int, int,
BaseNCodec$Context) where default case is missing
In this case for [codec], it looks like the code was carefully
constructed
and that no default clause is needed.
In these cases for any component, this FindBugs issue feels like a
style
issue, is it worth changing the code to add a default clause like:
default:
// ok to fall through for other values.
break;
Or does this feel like noise to you all?
In Math, there is this kind of code:
---CUT---
switch (method) {
case ILLINOIS:
f0 *= 0.5;
break;
case PEGASUS:
f0 *= f1 / (f1 + fx);
break;
case REGULA_FALSI:
// Detect early that algorithm is stuck, instead
of
// waiting
// for the maximum number of iterations to be
exceeded.
if (x == x1) {
throw new ConvergenceException();
}
break;
default:
// Should never happen.
throw new MathInternalError();
}
---CUT---
What about the opposite case:
We do not care about the other values than the ones in each switch case.
Wouldn't adding an empty default be enough to mean that (and silence
FindBugs)?
Regards,
Gilles
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
Re: [all] FindBugs' Switch statement found in class.method where default case is missing
I understand your point about the code having to be descriptive about why
the clause is not present, which is why I suggested using checkstyle, as
it allows for configuration of a comment which skips that check.
However, IMHO your point is valid about having an assertion/error such
that future changes don't break the logic.
Liv
Liviu Tudor
E: liviu.tu...@gmail.com
C: +1 650-2833815 (USA)
M: +44 (0)7591540313 (UK, Europe)
W: http://about.me/liviutudor
Skype: liviutudor
I'm nobody, nobody's perfect -- therefore I'm perfect!
On 05/09/2012 13:02, sebb seb...@gmail.com wrote:
Seems to me there are several reasons why the default case may have
been omitted:
1) It was accidentally omitted.
In this case, add the required clause.
2) It was omitted because nothing needs to be done.
In this case, this needs to be documented; the easiest way is to add:
default:
break; // nothing needs to be done here
3) It was omitted because it cannot happen
In this case, add an assertion or throw an error.
This is so that a future change which invalidates the assumption is
not overlooked, but is caught.
It's important that the person reading the code knows that the switch
block is complete.
So fixing the problem by changing CheckStyle or Findbugs configuration
files is not sufficient; there must be at least a comment in the code.
On 5 September 2012 01:04, Liviu Tudor liviu.tu...@gmail.com wrote:
Hi guys,
I am looking at this from a different perspective: the same check can be
performed using checkstyle
(http://checkstyle.sourceforge.net/config_coding.html#MissingSwitchDefaul
t)
as well as FindBugs. So if this is a valid case where there is no need
for a default branch, we could perhaps simply use a configured
checkstyle
comment to instruct checkstyle that this check doesn't need to be
performed in this case?
If this definitely needs FindBugs, I know that FindBugs supports
annotations, so it might be worth looking into that, though I don't know
off the top of my head if there is one available for default missing in
a
switch.
Liv
Liviu Tudor
E: liviu.tu...@gmail.com
C: +1 650-2833815 (USA)
M: +44 (0)7591540313 (UK, Europe)
W: http://about.me/liviutudor
Skype: liviutudor
I'm nobody, nobody's perfect -- therefore I'm perfect!
On 04/09/2012 16:02, Gilles Sadowski gil...@harfang.homelinux.org
wrote:
FindBugs can give warnings like:
Switch statement found in
org.apache.commons.codec.binary.Base32.decode(byte[], int, int,
BaseNCodec$Context) where default case is missing
In this case for [codec], it looks like the code was carefully
constructed
and that no default clause is needed.
In these cases for any component, this FindBugs issue feels like a
style
issue, is it worth changing the code to add a default clause like:
default:
// ok to fall through for other values.
break;
Or does this feel like noise to you all?
In Math, there is this kind of code:
---CUT---
switch (method) {
case ILLINOIS:
f0 *= 0.5;
break;
case PEGASUS:
f0 *= f1 / (f1 + fx);
break;
case REGULA_FALSI:
// Detect early that algorithm is stuck, instead
of
// waiting
// for the maximum number of iterations to be
exceeded.
if (x == x1) {
throw new ConvergenceException();
}
break;
default:
// Should never happen.
throw new MathInternalError();
}
---CUT---
What about the opposite case:
We do not care about the other values than the ones in each switch
case.
Wouldn't adding an empty default be enough to mean that (and silence
FindBugs)?
Regards,
Gilles
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
Re: [all] FindBugs' Switch statement found in class.method where default case is missing
On Wed, Sep 5, 2012 at 4:02 PM, sebb seb...@gmail.com wrote:
Seems to me there are several reasons why the default case may have
been omitted:
1) It was accidentally omitted.
In this case, add the required clause.
2) It was omitted because nothing needs to be done.
In this case, this needs to be documented; the easiest way is to add:
default:
break; // nothing needs to be done here
3) It was omitted because it cannot happen
In this case, add an assertion or throw an error.
This is so that a future change which invalidates the assumption is
not overlooked, but is caught.
It's important that the person reading the code knows that the switch
block is complete.
So fixing the problem by changing CheckStyle or Findbugs configuration
files is not sufficient; there must be at least a comment in the code.
Good guideline Sebb. Now, we need to circle back to our specific [codec]
cases... :)
Gary
On 5 September 2012 01:04, Liviu Tudor liviu.tu...@gmail.com wrote:
Hi guys,
I am looking at this from a different perspective: the same check can be
performed using checkstyle
(
http://checkstyle.sourceforge.net/config_coding.html#MissingSwitchDefault)
as well as FindBugs. So if this is a valid case where there is no need
for a default branch, we could perhaps simply use a configured checkstyle
comment to instruct checkstyle that this check doesn't need to be
performed in this case?
If this definitely needs FindBugs, I know that FindBugs supports
annotations, so it might be worth looking into that, though I don't know
off the top of my head if there is one available for default missing in a
switch.
Liv
Liviu Tudor
E: liviu.tu...@gmail.com
C: +1 650-2833815 (USA)
M: +44 (0)7591540313 (UK, Europe)
W: http://about.me/liviutudor
Skype: liviutudor
I'm nobody, nobody's perfect -- therefore I'm perfect!
On 04/09/2012 16:02, Gilles Sadowski gil...@harfang.homelinux.org
wrote:
FindBugs can give warnings like:
Switch statement found in
org.apache.commons.codec.binary.Base32.decode(byte[], int, int,
BaseNCodec$Context) where default case is missing
In this case for [codec], it looks like the code was carefully
constructed
and that no default clause is needed.
In these cases for any component, this FindBugs issue feels like a
style
issue, is it worth changing the code to add a default clause like:
default:
// ok to fall through for other values.
break;
Or does this feel like noise to you all?
In Math, there is this kind of code:
---CUT---
switch (method) {
case ILLINOIS:
f0 *= 0.5;
break;
case PEGASUS:
f0 *= f1 / (f1 + fx);
break;
case REGULA_FALSI:
// Detect early that algorithm is stuck, instead
of
// waiting
// for the maximum number of iterations to be
exceeded.
if (x == x1) {
throw new ConvergenceException();
}
break;
default:
// Should never happen.
throw new MathInternalError();
}
---CUT---
What about the opposite case:
We do not care about the other values than the ones in each switch
case.
Wouldn't adding an empty default be enough to mean that (and silence
FindBugs)?
Regards,
Gilles
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
--
E-Mail: garydgreg...@gmail.com | ggreg...@apache.org
JUnit in Action, 2nd Ed: http://goog_1249600977http://bit.ly/ECvg0
Spring Batch in Action: http://s.apache.org/HOqhttp://bit.ly/bqpbCK
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory
Re: [all] FindBugs' Switch statement found in class.method where default case is missing
Hi.
FindBugs can give warnings like:
Switch statement found in
org.apache.commons.codec.binary.Base32.decode(byte[], int, int,
BaseNCodec$Context) where default case is missing
In this case for [codec], it looks like the code was carefully constructed
and that no default clause is needed.
In these cases for any component, this FindBugs issue feels like a style
issue, is it worth changing the code to add a default clause like:
default:
// ok to fall through for other values.
break;
Or does this feel like noise to you all?
In Math, there is this kind of code:
---CUT---
switch (method) {
case ILLINOIS:
f0 *= 0.5;
break;
case PEGASUS:
f0 *= f1 / (f1 + fx);
break;
case REGULA_FALSI:
// Detect early that algorithm is stuck, instead of
// waiting
// for the maximum number of iterations to be exceeded.
if (x == x1) {
throw new ConvergenceException();
}
break;
default:
// Should never happen.
throw new MathInternalError();
}
---CUT---
Gilles
Gary
--
E-Mail: garydgreg...@gmail.com | ggreg...@apache.org
JUnit in Action, 2nd Ed: http://goog_1249600977http://bit.ly/ECvg0
Spring Batch in Action: http://s.apache.org/HOqhttp://bit.ly/bqpbCK
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
Re: [all] FindBugs' Switch statement found in class.method where default case is missing
Hi Gary, IMHO FindBugs is supposed to point you at code fragments that potentially could cause subtle bugs. If say that the code in codec is carefully constructed and everything is backed up by junit tests, I'd say a default clause is nosy and doesn't add anything. OTOH if you can not see that no default clause is required at first sight, I would prefer a default clause in favor of a code comment. Benedikt 2012/9/4 Gary Gregory garydgreg...@gmail.com: Hi All: FindBugs can give warnings like: Switch statement found in org.apache.commons.codec.binary.Base32.decode(byte[], int, int, BaseNCodec$Context) where default case is missing In this case for [codec], it looks like the code was carefully constructed and that no default clause is needed. In these cases for any component, this FindBugs issue feels like a style issue, is it worth changing the code to add a default clause like: default: // ok to fall through for other values. break; Or does this feel like noise to you all? Gary -- E-Mail: garydgreg...@gmail.com | ggreg...@apache.org JUnit in Action, 2nd Ed: http://goog_1249600977http://bit.ly/ECvg0 Spring Batch in Action: http://s.apache.org/HOqhttp://bit.ly/bqpbCK Blog: http://garygregory.wordpress.com Home: http://garygregory.com/ Tweet! http://twitter.com/GaryGregory - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [all] FindBugs' Switch statement found in class.method where default case is missing
Something like: throw new IllegalArgumentException(This should never happen because we are so smart we thought of every possibility in our case statement.); would suffice :) On Tue, Sep 4, 2012 at 9:02 AM, Benedikt Ritter benerit...@gmail.com wrote: Hi Gary, IMHO FindBugs is supposed to point you at code fragments that potentially could cause subtle bugs. If say that the code in codec is carefully constructed and everything is backed up by junit tests, I'd say a default clause is nosy and doesn't add anything. OTOH if you can not see that no default clause is required at first sight, I would prefer a default clause in favor of a code comment. Benedikt 2012/9/4 Gary Gregory garydgreg...@gmail.com: Hi All: FindBugs can give warnings like: Switch statement found in org.apache.commons.codec.binary.Base32.decode(byte[], int, int, BaseNCodec$Context) where default case is missing In this case for [codec], it looks like the code was carefully constructed and that no default clause is needed. In these cases for any component, this FindBugs issue feels like a style issue, is it worth changing the code to add a default clause like: default: // ok to fall through for other values. break; Or does this feel like noise to you all? Gary -- E-Mail: garydgreg...@gmail.com | ggreg...@apache.org JUnit in Action, 2nd Ed: http://goog_1249600977http://bit.ly/ECvg0 Spring Batch in Action: http://s.apache.org/HOqhttp://bit.ly/bqpbCK Blog: http://garygregory.wordpress.com Home: http://garygregory.com/ Tweet! http://twitter.com/GaryGregory - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [all] FindBugs' Switch statement found in class.method where default case is missing
Hello, 2012/9/4 James Carman ja...@carmanconsulting.com: Something like: throw new IllegalArgumentException(This should never happen because we are so smart we thought of every possibility in our case statement.); would suffice :) Not that it really matters, since this is never going to occur, but I think Bloch throws AssertionError in such cases. I think that IllegalStateException would be more appropriate in this case. But it really is for the sake of the argument anyway... Sébastien - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [all] FindBugs' Switch statement found in class.method where default case is missing
On Tue, Sep 4, 2012 at 9:01 AM, Gilles Sadowski
gil...@harfang.homelinux.org wrote:
Hi.
FindBugs can give warnings like:
Switch statement found in
org.apache.commons.codec.binary.Base32.decode(byte[], int, int,
BaseNCodec$Context) where default case is missing
In this case for [codec], it looks like the code was carefully
constructed
and that no default clause is needed.
In these cases for any component, this FindBugs issue feels like a style
issue, is it worth changing the code to add a default clause like:
default:
// ok to fall through for other values.
break;
Or does this feel like noise to you all?
In Math, there is this kind of code:
---CUT---
switch (method) {
case ILLINOIS:
f0 *= 0.5;
break;
case PEGASUS:
f0 *= f1 / (f1 + fx);
break;
case REGULA_FALSI:
// Detect early that algorithm is stuck, instead of
// waiting
// for the maximum number of iterations to be exceeded.
if (x == x1) {
throw new ConvergenceException();
}
break;
default:
// Should never happen.
throw new MathInternalError();
}
---CUT---
What about the opposite case:
We do not care about the other values than the ones in each switch case.
Gary
Gilles
Gary
--
E-Mail: garydgreg...@gmail.com | ggreg...@apache.org
JUnit in Action, 2nd Ed: http://goog_1249600977http://bit.ly/ECvg0
Spring Batch in Action: http://s.apache.org/HOqhttp://bit.ly/bqpbCK
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
--
E-Mail: garydgreg...@gmail.com | ggreg...@apache.org
JUnit in Action, 2nd Ed: http://goog_1249600977http://bit.ly/ECvg0
Spring Batch in Action: http://s.apache.org/HOqhttp://bit.ly/bqpbCK
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory
Re: [all] FindBugs' Switch statement found in class.method where default case is missing
I wasn't necessarily saying that we should always use IllegalArgumentException (although it can be applicable if the thing being switched upon is an argument to the method). The idea was that we could throw an exception of some sort in our default clause. On Tue, Sep 4, 2012 at 10:44 AM, Sébastien Brisard sebastien.bris...@m4x.org wrote: Hello, 2012/9/4 James Carman ja...@carmanconsulting.com: Something like: throw new IllegalArgumentException(This should never happen because we are so smart we thought of every possibility in our case statement.); would suffice :) Not that it really matters, since this is never going to occur, but I think Bloch throws AssertionError in such cases. I think that IllegalStateException would be more appropriate in this case. But it really is for the sake of the argument anyway... Sébastien - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [all] FindBugs' Switch statement found in class.method where default case is missing
Sorry James, you misunderstood me. My point was just that Bloch seems to be consistently using the same exception, namely AssertionError, but that I'd rather use IllegalStateException. Of course, in some cases IllegalArgumentException would make sense. S 2012/9/4 James Carman ja...@carmanconsulting.com: I wasn't necessarily saying that we should always use IllegalArgumentException (although it can be applicable if the thing being switched upon is an argument to the method). The idea was that we could throw an exception of some sort in our default clause. On Tue, Sep 4, 2012 at 10:44 AM, Sébastien Brisard sebastien.bris...@m4x.org wrote: Hello, 2012/9/4 James Carman ja...@carmanconsulting.com: Something like: throw new IllegalArgumentException(This should never happen because we are so smart we thought of every possibility in our case statement.); would suffice :) Not that it really matters, since this is never going to occur, but I think Bloch throws AssertionError in such cases. I think that IllegalStateException would be more appropriate in this case. But it really is for the sake of the argument anyway... Sébastien - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [all] FindBugs' Switch statement found in class.method where default case is missing
FindBugs can give warnings like:
Switch statement found in
org.apache.commons.codec.binary.Base32.decode(byte[], int, int,
BaseNCodec$Context) where default case is missing
In this case for [codec], it looks like the code was carefully
constructed
and that no default clause is needed.
In these cases for any component, this FindBugs issue feels like a style
issue, is it worth changing the code to add a default clause like:
default:
// ok to fall through for other values.
break;
Or does this feel like noise to you all?
In Math, there is this kind of code:
---CUT---
switch (method) {
case ILLINOIS:
f0 *= 0.5;
break;
case PEGASUS:
f0 *= f1 / (f1 + fx);
break;
case REGULA_FALSI:
// Detect early that algorithm is stuck, instead of
// waiting
// for the maximum number of iterations to be exceeded.
if (x == x1) {
throw new ConvergenceException();
}
break;
default:
// Should never happen.
throw new MathInternalError();
}
---CUT---
What about the opposite case:
We do not care about the other values than the ones in each switch case.
Wouldn't adding an empty default be enough to mean that (and silence
FindBugs)?
Regards,
Gilles
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
Re: [all] FindBugs' Switch statement found in class.method where default case is missing
Hi guys,
I am looking at this from a different perspective: the same check can be
performed using checkstyle
(http://checkstyle.sourceforge.net/config_coding.html#MissingSwitchDefault)
as well as FindBugs. So if this is a valid case where there is no need
for a default branch, we could perhaps simply use a configured checkstyle
comment to instruct checkstyle that this check doesn't need to be
performed in this case?
If this definitely needs FindBugs, I know that FindBugs supports
annotations, so it might be worth looking into that, though I don't know
off the top of my head if there is one available for default missing in a
switch.
Liv
Liviu Tudor
E: liviu.tu...@gmail.com
C: +1 650-2833815 (USA)
M: +44 (0)7591540313 (UK, Europe)
W: http://about.me/liviutudor
Skype: liviutudor
I'm nobody, nobody's perfect -- therefore I'm perfect!
On 04/09/2012 16:02, Gilles Sadowski gil...@harfang.homelinux.org
wrote:
FindBugs can give warnings like:
Switch statement found in
org.apache.commons.codec.binary.Base32.decode(byte[], int, int,
BaseNCodec$Context) where default case is missing
In this case for [codec], it looks like the code was carefully
constructed
and that no default clause is needed.
In these cases for any component, this FindBugs issue feels like a
style
issue, is it worth changing the code to add a default clause like:
default:
// ok to fall through for other values.
break;
Or does this feel like noise to you all?
In Math, there is this kind of code:
---CUT---
switch (method) {
case ILLINOIS:
f0 *= 0.5;
break;
case PEGASUS:
f0 *= f1 / (f1 + fx);
break;
case REGULA_FALSI:
// Detect early that algorithm is stuck, instead
of
// waiting
// for the maximum number of iterations to be
exceeded.
if (x == x1) {
throw new ConvergenceException();
}
break;
default:
// Should never happen.
throw new MathInternalError();
}
---CUT---
What about the opposite case:
We do not care about the other values than the ones in each switch case.
Wouldn't adding an empty default be enough to mean that (and silence
FindBugs)?
Regards,
Gilles
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
