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