On 20 May 2013 10:06, Julien Vermillard <jvermill...@gmail.com> wrote:
> Hi,
> comments inline
> On Mon, May 20, 2013 at 10:59 AM, Emmanuel Lécharny <elecha...@gmail.com>
> wrote:
>> Le 5/19/13 11:18 PM, Raphaël Barazzutti a écrit :
>>>
>>> I'm currently working fixing some issues in the delimited module of codec
>>> and the serialization modules.
>>>
>>> Currently some lines in my codes are generating warnings in Sonar.
>>
>> You are not alone here :) I fixed many warnings last saturday in the train.
>>>
>>> I'd like to know if there is a specific Sonar configuration file
>>> recommended for the MINA project.
>>
>> I think so, Julien ?
>
> I think it's running the dumb default config.
>
>>>
>>> And then I'd like to know what are your recommandations for a warning I
>>> currently encounter quite often while playing with RawInt32 and VarInt is
>>> the "'x' is a magic number" warning (by example when doing a shift of 8
>>> bits to the left/right).
>>>
>>> In such a situations I see some different ways of handling that warning :
>>> 1) do nothing, considering this warning as excessive
>> In mst of the case, yes.
>>
>> for instance, I don't see why we should fix things like :
>>
>> return ((input.get() & 0xff) << 24) | ((input.get() & 0xff)
>> << 16) | ((input.get() & 0xff) << 8)
>
> I think this rule apply for dumb J2E business code, we are too low
> level for that :)
Code is never too low-level for good comments and documentation.
Once I found several bugs in assembler code by replacing all the magic
numbers with symbolic constants.
Took a while, but they would have been very hard to find otherwise and
probably would not have surfaced until live running.
> I'm not going to create "FF = 0xff;" constant :)
That would be silly - it achieves nothing.
But why is the mask exactly 8 bits?
That would be documented by using
static final int MASK_8_BITS = 0xFF;
The point about replacing magic numbers with constants is that it
documents them.
And if something which happens to have the value 3 currently needs to
change to 5, you know which instances of 3 need to be changed.
It does force one to think about what each number means, which is a good thing.
You might find a bug - e.g. why is days[365] created?
Oops - what about leap years?
But it only has to be done once, whereas untreated magic numbers have
to be interpreted every time a new maintainer reads the code.
[And sometimes the same maintainer, if they have been away a while.]
It will take a bit longer initially.
But I contend that there will be ongoing savings in maintenance and debugging.
>>> 2) create a constants for all that cases
>>
>> I some case, that could be good :
>>
>> if ((input.get(3) & 0x80) != 0) {
>>
>> The '3' should remain, the 0x80 *could* be replaced by a constant :
Why keep 3?
What does it mean?
>>
>> public statif final int HIGH_BIT_MASK = 0x80;
>>
>
> That could be nice, but it's not socking me, I read bit flipping code
> fluently ;)
Not everyone does ...
But in this case I'd consider creating a private helper method if
there is much bit checking going on.
>> but this is just a possibility.
>>> 3) ask Sonar to ignore that line with a //nosonar
>> No. Sonar can be re-configured, and I don't see any added value to
>> inject such things in the code.
>>
>> One more thing :
>>
>> code like
>>
>> return ((input.get() & 0xff) << 24) | ((input.get() &
>> 0xff) << 16) | ((input.get() & 0xff) << 8)
>> | ((input.get() & 0xff));
>>
>> where input is a ByteBuffer can be replaced with :
>>
>> return input.getInt()
>>
>>
>> (in RawInt32)
> Yes and no, if you want to have a unsigned int :
>
> long unsignedInt = input.getInt() &0xFFFFFFFFL; no ?
>
>
> Julien
