commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gary Gregory <garydgreg...@gmail.com>
Subject Re: [all] FindBugs' Switch statement found in class.method where default case is missing
Date Wed, 05 Sep 2012 20:36:41 GMT
On Wed, Sep 5, 2012 at 4:02 PM, 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.
>

Good guideline Sebb. Now, we need to circle back to our specific [codec]
cases... :)

Gary


>
> 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#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.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
>
>


-- 
E-Mail: garydgregory@gmail.com | ggregory@apache.org
JUnit in Action, 2nd Ed: <http://goog_1249600977>http://bit.ly/ECvg0
Spring Batch in Action: <http://s.apache.org/HOq>http://bit.ly/bqpbCK
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message