You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by Raphaël Barazzutti <ra...@gmail.com> on 2013/05/19 23:18:20 UTC
Sonar
Hi all!
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.
I'd like to know if there is a specific Sonar configuration file
recommended for the MINA project.
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
2) create a constants for all that cases
3) ask Sonar to ignore that line with a //nosonar
Which approach would you recommend? Another idea?
Regards,
Raphaël
Re: Sonar
Posted by Raphaël Barazzutti <ra...@gmail.com>.
Thanks for all your comments :)
>
> > 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
>
I agree, I can switch and use the getInt() method given by the ByteBuffer.
I just have to ensure that the endianness is respected with order().
Re: Sonar
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 7/16/13 3:02 AM, sebb a écrit :
> On 20 May 2013 10:06, Julien Vermillard <jv...@gmail.com> wrote:
>> Hi,
>> comments inline
>> On Mon, May 20, 2013 at 10:59 AM, Emmanuel Lécharny <el...@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.
Absolutly ! Now, there are some common patterns that are recurrent, and
I don't see the added value for long comments when the code is
self-explicit.
In this very case (and there is a missing line, : "| input.get() &
0xff)", everybody that has spend more than a year coding in Java will
know that it decode an integer from an array of bytes.
> 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.
Yep.
>
> But why is the mask exactly 8 bits?
> That would be documented by using
>
> static final int MASK_8_BITS = 0xFF;
I agree. In Java, it's also important because there is a *big difference
between 0xFF and 0x00FF, which must be used when operating on integers
(because of the sign it that can be transmitted). There are rare cases
where you want to use 0x00FF instead of 0xFF, and it's easier to use
constants for that :
static final int MASK_8_BITS = 0xFF;
static final int UNSIGNED_MASK_8_BITS = 0x00FF;
>>>> 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?
It's absolutely clear in the context. Replacing '3' by a constant here
would be silly. Sadly, the extracted line does not shows why, but again,
the pb is the missing context, not the missing constant...
>
>>> 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.
Here, I'm balanced. It's a PITA to check what a HGH_BIT_MASK means, when
you obviously know what it's all about when you read 0x80. OTOH, it's
often an even more PITA to have a tedious bug just because the
HIGH_BIT_MASK is supposed to have been changed to 0x8000, everywhere
except in *this* portion of the code... (I already experienced suhc a
pain...)
--
Regards,
Cordialement,
Emmanuel Lécharny
www.iktek.com
Re: Sonar
Posted by sebb <se...@gmail.com>.
On 20 May 2013 10:06, Julien Vermillard <jv...@gmail.com> wrote:
> Hi,
> comments inline
> On Mon, May 20, 2013 at 10:59 AM, Emmanuel Lécharny <el...@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
Re: Sonar
Posted by Julien Vermillard <jv...@gmail.com>.
Hi,
comments inline
On Mon, May 20, 2013 at 10:59 AM, Emmanuel Lécharny <el...@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 :)
I'm not going to create "FF = 0xff;" constant :)
>> 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 :
>
> public statif final int HIGH_BIT_MASK = 0x80;
>
That could be nice, but it's not socking me, I read bit flipping code
fluently ;)
> 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
Re: Sonar
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 5/19/13 11:18 PM, Raphaël Barazzutti a écrit :
> Hi all!
Hi Raphaël !
>
> 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 ?
>
> 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)
> 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 :
public statif final int HIGH_BIT_MASK = 0x80;
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)
--
Regards,
Cordialement,
Emmanuel Lécharny
www.iktek.com