patternjavaMinor
URLEncoder implementation
Viewed 0 times
implementationurlencoderstackoverflow
Problem
I'm looking for feedback on my
```
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.catalina.util;
import java.nio.Buffer;
import java.nio.ByteBuffer;
import java.nio.CharBuffer;
import java.nio.charset.Charset;
import java.nio.charset.CharsetEncoder;
import java.nio.charset.CoderResult;
import java.nio.charset.CodingErrorAction;
import java.util.BitSet;
import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.BlockingQueue;
/**
*
*/
public class URLEncoder {
/**
* This constant determines how many characters
* the encoder can process at once
*/
private static final int CHAR_BUFFER_SIZE = 8;
/**
* On one hand, the output buffer should be large
* enough to avoid most overflow conditions when
* possible. On the other hand too large buffer
* is a waste of space
*/
private static final int BYTE_BUFFER_SIZE = CHAR_BUFFER_SIZE * 2;
/**
* How many enco
URLEncoder implementation, which is a drop-in replacement for Apache Tomcat's URLEncoder. It was completely ignored by the project committers (and I got no feedback why) and I'm curious if it's just that bad, or I failed to communicate why it's better than the original one.```
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.catalina.util;
import java.nio.Buffer;
import java.nio.ByteBuffer;
import java.nio.CharBuffer;
import java.nio.charset.Charset;
import java.nio.charset.CharsetEncoder;
import java.nio.charset.CoderResult;
import java.nio.charset.CodingErrorAction;
import java.util.BitSet;
import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.BlockingQueue;
/**
*
*/
public class URLEncoder {
/**
* This constant determines how many characters
* the encoder can process at once
*/
private static final int CHAR_BUFFER_SIZE = 8;
/**
* On one hand, the output buffer should be large
* enough to avoid most overflow conditions when
* possible. On the other hand too large buffer
* is a waste of space
*/
private static final int BYTE_BUFFER_SIZE = CHAR_BUFFER_SIZE * 2;
/**
* How many enco
Solution
Regarding the fact that it was ignored: Of course we don't know the
whole background, but you already mentioned a lot of possible reasons
yourself. I just want to highlight that "a lot more complicated" is
actually an excellent rejection reason since this is a conceptually very
simple component (so their focus might be on completely different features)
and understanding and maintaining a much more complicated piece of code
is a pretty big burden to drop on any team.
If you haven't done it already it would need a whole lot of test cases
to ensure that it works properly under all circumstances and some proof
of the performance impact for (semi-)realistic use cases(!) for me to
consider it. Point being that its superior performance might just be
drowned out by a whole lot of other effects.
If that's all not the case, maybe you just need to persist.
IMO it's nice and gives me a few ideas for another project, so thanks
as well as good luck!
inner
have some typos. (Yes, nitpicky, just saying.)
a summary or be removed.
whole background, but you already mentioned a lot of possible reasons
yourself. I just want to highlight that "a lot more complicated" is
actually an excellent rejection reason since this is a conceptually very
simple component (so their focus might be on completely different features)
and understanding and maintaining a much more complicated piece of code
is a pretty big burden to drop on any team.
If you haven't done it already it would need a whole lot of test cases
to ensure that it works properly under all circumstances and some proof
of the performance impact for (semi-)realistic use cases(!) for me to
consider it. Point being that its superior performance might just be
drowned out by a whole lot of other effects.
If that's all not the case, maybe you just need to persist.
IMO it's nice and gives me a few ideas for another project, so thanks
as well as good luck!
- The
encodemethod is relatively big, consider splitting off the
inner
else branch if possible.- The docstrings are sometimes missing periods at the of sentences and
have some typos. (Yes, nitpicky, just saying.)
- The main docstring for the class is empty, that should definitely have
a summary or be removed.
Context
StackExchange Code Review Q#142428, answer score: 2
Revisions (0)
No revisions yet.