You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@myfaces.apache.org by Simon Lessard <si...@gmail.com> on 2008/10/16 21:50:30 UTC
[TRINIDAD] Shallowed ClassCastException in SortableModel
Hi all,
I'm working on an ADF Faces Rich Client atm and as some may know, that
library is based on Trinidad. Anyway, while debugging for a completely
different issue, I found out the following in SortableModel class:
private int _toRowIndex(Object rowKey)
{
if (rowKey == null)
return -1;
try
{
return ((Integer)rowKey).intValue();
}
catch (ClassCastException e)
{
_LOG.warning("INVALID_ROWKEY", new Object[]{rowKey ,
rowKey.getClass()});
_LOG.warning(e);
return -1;
}
}
I think we should change that to something like the following to not shallow
an Exception that's more expensive to create than an instanceof check. Note
that for some reason I get one such exception per request currently so this
is kind of annoying ;) :
private int _toRowIndex(Object rowKey)
{
if (rowKey == null)
{
return -1;
}
else if (rowKey instanceof Integer)
{
return ((Integer)rowKey).intValue();
}
else
{
_LOG.warning("INVALID_ROWKEY", new Object[]{rowKey ,
rowKey.getClass()});
return -1;
}
}
What do you think?
~ Simon
Re: [TRINIDAD] Shallowed ClassCastException in SortableModel
Posted by Simon Lessard <si...@gmail.com>.
propagating would be fine for me as well, anything but current behavior
actually.
~ Simon
On Thu, Oct 16, 2008 at 4:50 PM, Blake Sullivan
<bl...@oracle.com>wrote:
> Actually, I suspect that the correct solution is to allow the
> ClassCastException to propagate. I don't see how having random, non-Integer
> objects map to row-not-present is ever what the developer intended. This
> code looks like it was added to avoid exceptions during rendering, but I
> feel it is more important to find these problems during development.
>
> -- Blake Sullivan
>
> Simon Lessard said the following On 10/16/2008 12:50 PM PT:
>
> Hi all,
>
> I'm working on an ADF Faces Rich Client atm and as some may know, that
> library is based on Trinidad. Anyway, while debugging for a completely
> different issue, I found out the following in SortableModel class:
>
> private int _toRowIndex(Object rowKey)
> {
> if (rowKey == null)
> return -1;
>
> try
> {
> return ((Integer)rowKey).intValue();
> }
> catch (ClassCastException e)
> {
> _LOG.warning("INVALID_ROWKEY", new Object[]{rowKey ,
> rowKey.getClass()});
> _LOG.warning(e);
> return -1;
> }
> }
>
> I think we should change that to something like the following to not
> shallow an Exception that's more expensive to create than an instanceof
> check. Note that for some reason I get one such exception per request
> currently so this is kind of annoying ;) :
>
> private int _toRowIndex(Object rowKey)
> {
> if (rowKey == null)
> {
> return -1;
> }
> else if (rowKey instanceof Integer)
> {
> return ((Integer)rowKey).intValue();
> }
> else
> {
> _LOG.warning("INVALID_ROWKEY", new Object[]{rowKey ,
> rowKey.getClass()});
> return -1;
> }
> }
>
> What do you think?
>
>
> ~ Simon
>
>
>
Re: [TRINIDAD] Shallowed ClassCastException in SortableModel
Posted by Blake Sullivan <bl...@oracle.com>.
Actually, I suspect that the correct solution is to allow the
ClassCastException to propagate. I don't see how having random,
non-Integer objects map to row-not-present is ever what the developer
intended. This code looks like it was added to avoid exceptions during
rendering, but I feel it is more important to find these problems during
development.
-- Blake Sullivan
Simon Lessard said the following On 10/16/2008 12:50 PM PT:
> Hi all,
>
> I'm working on an ADF Faces Rich Client atm and as some may know, that
> library is based on Trinidad. Anyway, while debugging for a completely
> different issue, I found out the following in SortableModel class:
>
> private int _toRowIndex(Object rowKey)
> {
> if (rowKey == null)
> return -1;
>
> try
> {
> return ((Integer)rowKey).intValue();
> }
> catch (ClassCastException e)
> {
> _LOG.warning("INVALID_ROWKEY", new Object[]{rowKey ,
> rowKey.getClass()});
> _LOG.warning(e);
> return -1;
> }
> }
>
> I think we should change that to something like the following to not
> shallow an Exception that's more expensive to create than an
> instanceof check. Note that for some reason I get one such exception
> per request currently so this is kind of annoying ;) :
>
> private int _toRowIndex(Object rowKey)
> {
> if (rowKey == null)
> {
> return -1;
> }
> else if (rowKey instanceof Integer)
> {
> return ((Integer)rowKey).intValue();
> }
> else
> {
> _LOG.warning("INVALID_ROWKEY", new Object[]{rowKey ,
> rowKey.getClass()});
> return -1;
> }
> }
>
> What do you think?
>
>
> ~ Simon
Re: [TRINIDAD] Shallowed ClassCastException in SortableModel
Posted by Scott O'Bryan <da...@gmail.com>.
I hate API's like this, especially in private methods. If the thing
expects an Interger, why not just make the rowKey an integer? Grrrr.
Keeping things generic objects sucks.
Now off my soapbox. :)
+1 Simon, doing the instanceof is on orders of magnitude less expensive
then throwing an exception and assembling a stacktrace. It appears from
this code that a non integer is an acceptable (albeit unexpected)
codepath and we should never use exceptions for flow control. Good catch!
Scott
Simon Lessard wrote:
> Hi all,
>
> I'm working on an ADF Faces Rich Client atm and as some may know, that
> library is based on Trinidad. Anyway, while debugging for a completely
> different issue, I found out the following in SortableModel class:
>
> private int _toRowIndex(Object rowKey)
> {
> if (rowKey == null)
> return -1;
>
> try
> {
> return ((Integer)rowKey).intValue();
> }
> catch (ClassCastException e)
> {
> _LOG.warning("INVALID_ROWKEY", new Object[]{rowKey ,
> rowKey.getClass()});
> _LOG.warning(e);
> return -1;
> }
> }
>
> I think we should change that to something like the following to not
> shallow an Exception that's more expensive to create than an
> instanceof check. Note that for some reason I get one such exception
> per request currently so this is kind of annoying ;) :
>
> private int _toRowIndex(Object rowKey)
> {
> if (rowKey == null)
> {
> return -1;
> }
> else if (rowKey instanceof Integer)
> {
> return ((Integer)rowKey).intValue();
> }
> else
> {
> _LOG.warning("INVALID_ROWKEY", new Object[]{rowKey ,
> rowKey.getClass()});
> return -1;
> }
> }
>
> What do you think?
>
>
> ~ Simon