On 18 March 2013 17:07, Simone Tripodi <simonetrip...@apache.org> wrote: > I took care about it reverting the MagicNumber constants, see r1457865
Thanks, but I think you misunderstood me. I did not mean to say that constants should not be used for magic numbers. On the contrary, constants are good, especially if the number is used more than once. But the reason for the value must be documented (this is true even if a constant is not used) and a constant name should relate to its function. The checkstyle rule is useful in pointing out where such documentation is needed. I'll have a go at introducing function-specific constants shortly. > http://people.apache.org/~simonetripodi/ > http://simonetripodi.livejournal.com/ > http://twitter.com/simonetripodi > http://www.99soft.org/ > > > On Mon, Mar 18, 2013 at 12:12 PM, sebb <seb...@gmail.com> wrote: >> 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 >> > > --------------------------------------------------------------------- > 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
