1. Home
  2. Categories
  3. Side Bar WTF
  4. CheckStyle, round two + FindBugs

CheckStyle, round two + FindBugs

  • belgariontheking

    Here's the code that it's flagging

        /**
         * Return delimited list from a String array.
         *
         * @return the list of abort indicator text labels
         * @exception GilsException
         *                thrown if an error is encountered
         */
        public String getDelimitedList(String[] list, String delim) {
            if (list == null) {
                return "";
            }

            StringBuffer sb = new StringBuffer();
            for (int i = 0; i < list.length; i++) {
                sb.append(list[i]);
                if (i < list.length - 1) {
                    sb.append(delim);
                }
            }
            return sb.toString();
        }

    Here's the error out of CheckStyle:

    "Got an exception - java.lang.RuntimeException: Unable to get class information for @throws tag 'GilsException'."

    It's complaining that it can't find the GilsException (GILS is the name of the project).  Two One and a half problems:

    1) That error isn't declared.  Not needed, but in other cases in the app where it's declared in the comments, it's declared in the method header.

    2) It's not actually thrown!

    So this isn't a WTF on CheckStyle's part, but my predecessor.

     

    This next one is out of FindBugs, which is a bytecode analyzer, as opposed to a source code analyzer.

    Snipped up code:

            String[]][]] activityList = null;
    <snip>
                activityList = new String[2][rs.size() + 1];
    <snip>
                activityList[0][0] = "";
                activityList[1][0] = "All";
    <snip>
                return activityList;

    "Dead store to activityList in method <method name>"

    Umm, no.  Do you see the return there?  It's not a dead store if it's returned. It missed the variable being in the return multiple times.  

    Unfortunately, I have to get CIO approval to keep this code in, when there's nothing wrong with it.  I hope someone up the chain (before it gets to the CIO) understands that it's a problem in the analyzer tool.

    0
  • belgariontheking

    Wait, on the second one, do you think it could be flagging it because it's setting it to null and then initializing it later?

    That seems uncommonly lame if true.

    0
  • boomzilla
    ♿ (Parody)

    @belgariontheking said:

    Wait, on the second one, do you think it could be flagging it because it's setting it to null and then initializing it later?

    That seems uncommonly lame if true.


    Yes, I'd say it's bitching about the null assignment. In this case, it's pretty lame, but in general, potentially a valid issue. The question is, why declare in one place and initialize in another to begin with?

    0
  • X

    @belgariontheking said:

    Wait, on the second one, do you think it could be flagging it because it's setting it to null and then initializing it later?

    I vote for yes. Also, why isn't it flagging the StringBuffer? Something about deity and baby pets whenever you use a local StringBuffer instead of a StringBuilder.

    0
  • belgariontheking

    @boomzilla said:

    why declare in one place and initialize in another to begin with?
    In this case, it's declared outside a try that I didn't show you.  Inside the try is where it initializes, fills, and returns it.

    0
  • belgariontheking

    @Xyro said:

    why isn't it flagging the StringBuffer? Something about deity and baby pets whenever you use a local StringBuffer instead of a StringBuilder.
    I'm not one to question providence.

    0
  • P

    @belgariontheking said:

    @boomzilla said:
    why declare in one place and initialize in another to begin with?
    In this case, it's declared outside a try that I didn't show you.  Inside the try is where it initializes, fills, and returns it.

    But if it's returned inside the try, it can be declared inside the try. Minimise the scope and you can avoid the dead store, so it's not unreasonable to flag it up in pedantic mode (and IIRC from the previous thread, the config settings are pushing for pedantry).

    0
  • mott555

    @pjt33 said:

    @belgariontheking said:
    @boomzilla said:
    why declare in one place and initialize in another to begin with?
    In this case, it's declared outside a try that I didn't show you.  Inside the try is where it initializes, fills, and returns it.

    But if it's returned inside the try, it can be declared inside the try. Minimise the scope and you can avoid the dead store, so it's not unreasonable to flag it up in pedantic mode (and IIRC from the previous thread, the config settings are pushing for pedantry).

     

    Unless he needs access to the object within the catch or finally blocks.

     

    0
  • X

    @mott555 said:

    Unless he needs access to the object within the catch or finally blocks.

    ... In which case, it wouldn't necessarily be a dead store, and CheckStyle shouldn't be carping about it.

    0
  • J

    @mott555 said:

    @pjt33 said:

    @belgariontheking said:
    In this case, it's declared outside a try that I didn't show you.  Inside the try is where it initializes, fills, and returns it.

    But if it's returned inside the try, it can be declared inside the try. Minimise the scope and you can avoid the dead store, so it's not unreasonable to flag it up in pedantic mode (and IIRC from the previous thread, the config settings are pushing for pedantry).

     

    Unless he needs access to the object within the catch or finally blocks.

     

    Try moving or copying the return to outside the try.

     

    0
  • Matt_Westwood

    That first one, where the javadoc is inaccurate? That's not a WTF, it's not even a bug, it's a standard maintenance header-tidying task.

    0
Log in to reply