commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Liviu Tudor <liviu.tu...@gmail.com>
Subject Re: [all] FindBugs' Switch statement found in class.method where default case is missing
Date Wed, 05 Sep 2012 20:36:13 GMT
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.tudor@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" <sebbaz@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.tudor@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.tudor@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" <gilles@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-unsubscribe@commons.apache.org
>>>For additional commands, e-mail: dev-help@commons.apache.org
>>>
>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>For additional commands, e-mail: dev-help@commons.apache.org
>



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message