HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

URLEncoder implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
implementationurlencoderstackoverflow

Problem

I'm looking for feedback on my 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!

  • The encode method 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.