On 18 March 2013 10:45, Simone Tripodi <simonetrip...@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 <seb...@gmail.com> wrote: >> On 15 March 2013 16:20, <simonetrip...@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-unsubscr...@commons.apache.org >> For additional commands, e-mail: dev-h...@commons.apache.org >> > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
