commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: svn commit: r1457003 - /commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/util/mime/Base64Decoder.java
Date Mon, 18 Mar 2013 11:12:00 GMT
On 18 March 2013 10:45, Simone Tripodi <simonetripodi@apache.org> wrote:
> feel free to adjust them by yourself and make checkstyle silent about
> that rules - I am not so worried as you are about internal-use only
> classes which are intended to be used by fileupload and are not part
> of the APIs.

See my other post - it's still important that internal code
documentation is accurate.

Misleading constants should be avoided.

I will try and fix the code.

Please don't hide any further magic numbers just to make Checkstyle happy.
Ideally leave the warning in place for someone to fix.

> http://people.apache.org/~simonetripodi/
> http://simonetripodi.livejournal.com/
> http://twitter.com/simonetripodi
> http://www.99soft.org/
>
>
> On Mon, Mar 18, 2013 at 9:34 AM, sebb <sebbaz@gmail.com> wrote:
>> On 15 March 2013 16:20,  <simonetripodi@apache.org> wrote:
>>> Author: simonetripodi
>>> Date: Fri Mar 15 16:20:27 2013
>>> New Revision: 1457003
>>>
>>> URL: http://svn.apache.org/r1457003
>>> Log:
>>> checkstyle: magic numbers
>>
>> -1
>>
>> Although these changes don't affect the code, they are wrong.
>>
>> Just because the same number (e.g. 3) is used in several places, does
>> not mean that it represents the same value.
>> For example, 60 is the standard number of seconds in a minute, and
>> minutes in an hour, but code needs to use different names for them.
>>
>> The numbers now replaced by MASK_nBITS fields were used as shift
>> counts and index offsets, not masks.
>>
>> Different constants must be used for each different use, even if the
>> values are the same, and the name should reflect the meaning of the
>> constant.
>>
>>> Modified:
>>>     commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/util/mime/Base64Decoder.java
>>>
>>> Modified: commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/util/mime/Base64Decoder.java
>>> URL: http://svn.apache.org/viewvc/commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/util/mime/Base64Decoder.java?rev=1457003&r1=1457002&r2=1457003&view=diff
>>> ==============================================================================
>>> --- commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/util/mime/Base64Decoder.java
(original)
>>> +++ commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/util/mime/Base64Decoder.java
Fri Mar 15 16:20:27 2013
>>> @@ -25,6 +25,26 @@ import java.io.OutputStream;
>>>  final class Base64Decoder {
>>>
>>>      /**
>>> +     * Bytes per undecoded block.
>>> +     */
>>> +    private static final int BYTES_PER_UNENCODED_BLOCK = 3;
>>> +
>>> +    /**
>>> +     * 2 bits mask.
>>> +     */
>>> +    private static final int MASK_2BITS = 2;
>>> +
>>> +    /**
>>> +     * 4 bits mask.
>>> +     */
>>> +    private static final int MASK_4BITS = 4;
>>> +
>>> +    /**
>>> +     * 6 bits mask.
>>> +     */
>>> +    private static final int MASK_6BITS = 6;
>>> +
>>> +    /**
>>>       * Set up the encoding table.
>>>       */
>>>      private static final byte[] ENCODING_TABLE = {
>>> @@ -101,7 +121,7 @@ final class Base64Decoder {
>>>          }
>>>
>>>          int  i = off;
>>> -        int  finish = end - 4;
>>> +        int  finish = end - MASK_4BITS;
>>>
>>>          while (i < finish) {
>>>              while ((i < finish) && ignore((char) data[i])) {
>>> @@ -128,40 +148,40 @@ final class Base64Decoder {
>>>
>>>              b4 = DECODING_TABLE[data[i++]];
>>>
>>> -            out.write((b1 << 2) | (b2 >> 4));
>>> -            out.write((b2 << 4) | (b3 >> 2));
>>> -            out.write((b3 << 6) | b4);
>>> +            out.write((b1 << MASK_2BITS) | (b2 >> MASK_4BITS));
>>> +            out.write((b2 << MASK_4BITS) | (b3 >> MASK_2BITS));
>>> +            out.write((b3 << MASK_6BITS) | b4);
>>>
>>> -            outLen += 3;
>>> +            outLen += BYTES_PER_UNENCODED_BLOCK;
>>>          }
>>>
>>> -        if (data[end - 2] == PADDING) {
>>> -            b1 = DECODING_TABLE[data[end - 4]];
>>> -            b2 = DECODING_TABLE[data[end - 3]];
>>> +        if (data[end - MASK_2BITS] == PADDING) {
>>> +            b1 = DECODING_TABLE[data[end - MASK_4BITS]];
>>> +            b2 = DECODING_TABLE[data[end - BYTES_PER_UNENCODED_BLOCK]];
>>>
>>> -            out.write((b1 << 2) | (b2 >> 4));
>>> +            out.write((b1 << MASK_2BITS) | (b2 >> MASK_4BITS));
>>>
>>>              outLen += 1;
>>>          } else if (data[end - 1] == PADDING) {
>>> -            b1 = DECODING_TABLE[data[end - 4]];
>>> -            b2 = DECODING_TABLE[data[end - 3]];
>>> -            b3 = DECODING_TABLE[data[end - 2]];
>>> +            b1 = DECODING_TABLE[data[end - MASK_4BITS]];
>>> +            b2 = DECODING_TABLE[data[end - BYTES_PER_UNENCODED_BLOCK]];
>>> +            b3 = DECODING_TABLE[data[end - MASK_2BITS]];
>>>
>>> -            out.write((b1 << 2) | (b2 >> 4));
>>> -            out.write((b2 << 4) | (b3 >> 2));
>>> +            out.write((b1 << MASK_2BITS) | (b2 >> MASK_4BITS));
>>> +            out.write((b2 << MASK_4BITS) | (b3 >> MASK_2BITS));
>>>
>>> -            outLen += 2;
>>> +            outLen += MASK_2BITS;
>>>          } else {
>>> -            b1 = DECODING_TABLE[data[end - 4]];
>>> -            b2 = DECODING_TABLE[data[end - 3]];
>>> -            b3 = DECODING_TABLE[data[end - 2]];
>>> +            b1 = DECODING_TABLE[data[end - MASK_4BITS]];
>>> +            b2 = DECODING_TABLE[data[end - BYTES_PER_UNENCODED_BLOCK]];
>>> +            b3 = DECODING_TABLE[data[end - MASK_2BITS]];
>>>              b4 = DECODING_TABLE[data[end - 1]];
>>>
>>> -            out.write((b1 << 2) | (b2 >> 4));
>>> -            out.write((b2 << 4) | (b3 >> 2));
>>> -            out.write((b3 << 6) | b4);
>>> +            out.write((b1 << MASK_2BITS) | (b2 >> MASK_4BITS));
>>> +            out.write((b2 << MASK_4BITS) | (b3 >> MASK_2BITS));
>>> +            out.write((b3 << MASK_6BITS) | b4);
>>>
>>> -            outLen += 3;
>>> +            outLen += BYTES_PER_UNENCODED_BLOCK;
>>>          }
>>>
>>>          return outLen;
>>>
>>>
>>
>> ---------------------------------------------------------------------
>> 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


Mime
View raw message