You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by Emmanuel Lecharny <el...@gmail.com> on 2008/08/27 16:30:10 UTC
About IoFileradpter not being Abstract
Hi,
following my previous mail, I discovered that IoFilterAdapter is not
abstract may be because there is a cryptic method deep into
DefaultIoFilterChainBuilder which tries to instanciate this class :
public void setFilters(Map<String, ? extends IoFilter> filters) {
if (filters == null) {
throw new NullPointerException("filters");
}
if (!isOrderedMap(filters)) {
...
@SuppressWarnings("unchecked")
private boolean isOrderedMap(Map map) {
Class<?> mapType = map.getClass();
...
Random rand = new Random();
List<String> expectedNames = new ArrayList<String>();
IoFilter dummyFilter = new IoFilterAdapter();
<------------------------------this is here.
for (int i = 0; i < 65536; i ++) {
// WTF is this loop ???
String filterName;
do {
filterName = String.valueOf(rand.nextInt());
} while (newMap.containsKey(filterName));
newMap.put(filterName, dummyFilter);
expectedNames.add(filterName);
Iterator<String> it = expectedNames.iterator();
for (Object key: newMap.keySet()) {
if (!it.next().equals(key)) {
if (logger.isDebugEnabled()) {
logger.debug(
"The specified map didn't pass the
insertion " +
"order test after " + (i + 1) + " tries.");
}
return false;
}
}
}
Now, for what it worths, I'm really puzzled about this part of the code
: what is this suppose to do ??? There is _no_ explanation, this is
typically such a portion of code which makes it almost impossible to
maintain. I call it a one man show.
Plus I think it's totally useless. We can write defensive code, but at
some point, we have to stop, and think. The setFilters( Map ) method is
never called nowhere, and sounds like a YAGNI method to me. Why should
we allow a user to create his own map, assume that the user can be
malicious, and inject a cryptic method to check that the user didn't
broke something in his own code ? To avoid complex questions on the user
mailing list ?
IMHO, it's way better to remove such a piece of code, and also remove
the setFilters() method, as i'm pretty sure no one use it.
KISS should always be a moto when writing code, unless necessary.
--
--
cordialement, regards,
Emmanuel Lécharny
www.iktek.com
directory.apache.org
Re: About IoFileradpter not being Abstract
Posted by Julien Vermillard <jv...@archean.fr>.
On Wed, 27 Aug 2008 17:12:40 +0200
"Christian Migowski" <ch...@gmail.com> wrote:
> Hello,
>
> see comment below.
>
> 2008/8/27, Emmanuel Lecharny elecharny@gmail.com:
> >
> >
> > public void setFilters(Map<String, ? extends IoFilter> filters) {
> > if (filters == null) {
> > throw new NullPointerException("filters");
> > }
> > if (!isOrderedMap(filters)) {
> > ...
> >
> > @SuppressWarnings("unchecked")
> > private boolean isOrderedMap(Map map) {
> > Class<?> mapType = map.getClass();
> > ...
> > Random rand = new Random();
> > List<String> expectedNames = new ArrayList<String>();
> > IoFilter dummyFilter = new IoFilterAdapter();
> > <------------------------------this is here.
> > for (int i = 0; i < 65536; i ++) {
> > // WTF is this loop ???
> > String filterName;
> > do {
> > filterName = String.valueOf(rand.nextInt());
> > } while (newMap.containsKey(filterName));
>
>
> it generates a "unique" new key to be inserted in the new Map.
> Cleverly you omitted the comment that exists in the code:
>
> // Last resort: try to create a new instance and test if it maintains
> // the insertion order.
>
> you'll see why it is important to maintain the filter order that the
> user intended to have - even if HE/SHE made a mistake by choosing the
> wrong collection.
>
> Here is my opinion: It's this small bits and pieces and checks that
> try to cover _everything_ that make a framework worth using. If a
> framework just provides some sloppy checking and will fail
> mysteriously (depending on the filters, it could be quite difficult
> to fiddle out the mistake if the order in the map wasn't maintained),
> you are maybe better of doing it all by yourself. Just because
> something isn't understood doesn't mean it is a bad thing.
>
> christian
>
Hi,
I'll avoid the lyric part about what a framework is supposed to be :)
My preoccupation is to know if we need such a mechanism for changing the
map. Who use this setFilter method, and why ?
Julien
Re: About IoFileradpter not being Abstract
Posted by Christian Migowski <ch...@gmail.com>.
Hello,
see comment below.
2008/8/27, Emmanuel Lecharny elecharny@gmail.com:
>
>
> public void setFilters(Map<String, ? extends IoFilter> filters) {
> if (filters == null) {
> throw new NullPointerException("filters");
> }
> if (!isOrderedMap(filters)) {
> ...
>
> @SuppressWarnings("unchecked")
> private boolean isOrderedMap(Map map) {
> Class<?> mapType = map.getClass();
> ...
> Random rand = new Random();
> List<String> expectedNames = new ArrayList<String>();
> IoFilter dummyFilter = new IoFilterAdapter();
> <------------------------------this is here.
> for (int i = 0; i < 65536; i ++) {
> // WTF is this loop ???
> String filterName;
> do {
> filterName = String.valueOf(rand.nextInt());
> } while (newMap.containsKey(filterName));
it generates a "unique" new key to be inserted in the new Map. Cleverly you
omitted the comment that exists in the code:
// Last resort: try to create a new instance and test if it maintains
// the insertion order.
you'll see why it is important to maintain the filter order that the user
intended to have - even if HE/SHE made a mistake by choosing the wrong
collection.
Here is my opinion: It's this small bits and pieces and checks that try to
cover _everything_ that make a framework worth using. If a framework just
provides some sloppy checking and will fail mysteriously (depending on the
filters, it could be quite difficult to fiddle out the mistake if the order
in the map wasn't maintained), you are maybe better of doing it all by
yourself. Just because something isn't understood doesn't mean it is a bad
thing.
christian
newMap.put(filterName, dummyFilter);
> expectedNames.add(filterName);
>
> Iterator<String> it = expectedNames.iterator();
> for (Object key: newMap.keySet()) {
> if (!it.next().equals(key)) {
> if (logger.isDebugEnabled()) {
> logger.debug(
> "The specified map didn't pass the insertion
> " +
> "order test after " + (i + 1) + " tries.");
> }
> return false;
> }
> }
> }
>
>
> Now, for what it worths, I'm really puzzled about this part of the code :
> what is this suppose to do ??? There is _no_ explanation, this is typically
> such a portion of code which makes it almost impossible to maintain. I call
> it a one man show.
>
> Plus I think it's totally useless. We can write defensive code, but at some
> point, we have to stop, and think. The setFilters( Map ) method is never
> called nowhere, and sounds like a YAGNI method to me. Why should we allow a
> user to create his own map, assume that the user can be malicious, and
> inject a cryptic method to check that the user didn't broke something in his
> own code ? To avoid complex questions on the user mailing list ?
>
> IMHO, it's way better to remove such a piece of code, and also remove the
> setFilters() method, as i'm pretty sure no one use it.
>
> KISS should always be a moto when writing code, unless necessary.
>
> --
> --
> cordialement, regards,
> Emmanuel Lécharny
> www.iktek.com
> directory.apache.org
>
>
>