Monday, October 28, 2013

Trailing Comment Check false positives

Original investigation of Daniil Yroslavtsev:

Check: http://checkstyle.sourceforge.net/config_misc.html#TrailingComment
It has so much problem to report valid code so I do believe that it is switched off  in all companies configurations.

Other support for that Check: Trailing Comments .

But currently Trailing Comment check warns on the following valid cases:

1.

  A. 
  } catch (Exception e) { // SUPPRESS CHECKSTYLE IllegalCatchExtendedCheck now it part of logic

  B.
  } catch (Exception e) { // NOPMD

  C.
  } catch (Exception e) { // NOSONAR

2.
double emerging = 100 - regions.sumEstimate(
  "54138", // Not Applicable/Disclosed
  "19668", // United States
  "200005", // Canada
...
);
ATTENTION: it is not false-positive, as all this magic numbers
 should be moved to constants with proper names



3.


ReportValidationResult r = validationResult.get(i);
  sb.append(
    MessageFormat.format("  report_id={0} (co_id={1}), report_start$=''{2}'', report_end$=''{3}''"
     + ", period_start_date={4}, period_end_date={5}: "
     + "total percent on ''{6}'' is {7}% .... ",
     safeToPlainString(r.reportId), // 0
     safeToPlainString(r.companyId), // 1
     formatTimestamp(r.reportStartDate), // 2
     ...
     formatTimestamp(r.periodEndDate), // 7
     ...
     )

4.

  private static final Color[] COLORS = {             
            new Color(255, 128, 128), // pink
            new Color(255, 255, 128), // yellow             
            new Color(128, 255, 128), // green             
            new Color(128, 255, 255), // cian             
            new Color(255, 128, 192), // margent                      
            new Color(255, 128, 64), // brown
            new Color(0, 128, 128), // blue/gray
            new Color(128, 128, 255), // purple
            new Color(0, 128, 0), // dark green             
            new Color(128, 128, 0), // greenish/grayish     };

5
    record[++columnIndex] = geoAreaEntry.getKey(); // _NAME
    record[++columnIndex] = "unknown"; // _REGION
    record[++columnIndex] = "0"; // _WEIGHT
    record[++columnIndex] = "1"; // _CONF



To fix false-positives we can update Trailing Commentary Sonar check to ignore: //NOSONAR, //SUPPRESS CHECKSTYLE, //NOPMD, // formatter:off, // formatter:on commentaries (with all their possible variants) and other 'valid' comments by regexp in it`s configuration.

But I think, that Checkstyle Trailing Comment check should be disabled for all projects as too many comment formats can be considered as 'valid' and result 'ignore' regexp will be too complex and unreadable.

Monday, October 21, 2013

Checkstyle Check to detect System.out.println usage in java code

Examples of usage in code:
1.
import static java.lang.System.out;
...
out.println("hello!")

2.
System.out.println("hello");




Requirement:
- Analyse import for short name usage "import static java.lang.System.out;"

Possiblle name for Check: ForbidQualifierUsage

Options:
forbiddenFullQualifierNames : String List-  qualifier that we will search for (Example:"System.err.println")
ignoredClasses : Regex -  ignores (Example:"com.mycompany.console.Main")

Bad sides:
1) we can not distinguish methods by parameters so any user method with same name and without package usage will be false-positive if not all methods are investigated to detects overlap of static import in local declaration .

import static java.lang.System.setIn;


public static void setIn(InputStream in) {
    // just a method with similar signature that is compiled
    // if you comment out that method System.setIn will be used (tested in Intelij Idea)
}

public static void main(String... args) {
    setIn(null); // it is local if local method is defined
}

2) Any overloaded methods(same name but different parameters) will be false-positives or known limitations :) :

   System.out.println("hi", "hi")

https://docs.oracle.com/javase/specs/jls/se7/html/jls-7.html#jls-7.5.3

It is permissible for one single-static-import declaration to import several fields or types with the same name, or several methods with the same name and signature. 
So JDK is OK with multiple imports by single static import, so overloaded identifiers are ok.


Resolution:
So it is not complete solution, kind of same level of false positives as RegExp search on code approach.
Could we close eyes on that false-positives?
do you have other ideas and proposal?
https://github.com/checkstyle/checkstyle/issues?q=is%3Aopen+is%3Aissue
Or discussion mail-list - https://groups.google.com/forum/#!forum/checkstyle-devel

Friday, October 18, 2013

Thursday, October 17, 2013

Scala Books

List of books is here: http://www.scala-lang.org/documentation/books.html

I highly recommend to read "Programming in Scala Second edition" book , first edition could be find easily for free download.
This book is much better then "Scala for the Impatient" and "Scala in Depth" as it describe in details functional patterns and basics and have very good history of Scala language and show from where Scala grab ideas and why syntax enhancements are not in Java and .... .

Logging exception message in log event message

When exception happen and you want to log it and the right way to do it is

log.error("Processing is failed during phase {}", phase , e);

In log you will get something like this:
[main] ERROR test.Test - My message Error.
java.lang.RuntimeException:  Long long Message
 at test.Test.main(Test.java:10)

but this approach works fine for logging to output and file and have one problem with reporting to SysLog.
SysLog is not a java logger and it teat each line as separate event so printing stacktrace of exception will result into numerous event on SysLog level - so to resolve problem we use "throwableExcluded=true" , but we loose exception message in same time and it do contains useful information .
Exception message does not disclose reason of problem but it allow to quickly categorize problem and some time it is all that you need to fix a problem.

Example:
        <appender name="SYSLOG" class="ch.qos.logback.classic.net.SyslogAppender">
                <facility>LOCAL1</facility>
                <syslogHost>localhost</syslogHost>
                <suffixPattern>[%d${common.log.date.format}] [%t] [%c{0}] [%p]: %m%n</suffixPattern>
                <throwableExcluded>true</throwableExcluded>
                <filter class="ch.qos.logback.classic.filter.ThresholdFilter">
                      <!-- deny all events with a level below ERROR -->
                      <level>ERROR</level>
                </filter>                                                                                                                            
        </appender>


Alternative way of logging - add Exception message to message of event.

logger.error("Handler execution resulted in exception: " + e.getMessage(), e);
log.error("Processing is failed during phase {} with exception message='{}' ", phase, e.getMessage(), e);

result in message duplicate(see bold text) in file and system output that complicate log reading. This example is not very representative but imagine long message like Spring give on exception.
Example:
[main] ERROR test.Test - My message Error: Long long Message
java.lang.RuntimeException:  Long long Message
 at test.Test.main(Test.java:10)

but this will resolve problem of SyslogAppender and with "throwableExcluded"=true Syslog will have informative message.
Second approach is highly NOT recommended, as it brake nature of log events in java and do duplication of error details.

Throwable class in Java have getMessage() method:  http://docs.oracle.com/javase/7/docs/api/java/lang/Throwable.html

Proposal:
So we need to update SyslogAppender to have "throwableMessageIncluded" or "throwableMessageAppended" parameter that will attach to Event message exception message.
Posted as - http://mailman.qos.ch/pipermail/logback-dev/2013-October/009240.html

Logback resources:
Discussion thread : http://mailman.qos.ch/pipermail/logback-dev/2013-October/009240.html,
or create issue: http://jira.qos.ch/browse
sources only: https://github.com/qos-ch/logback

Monday, October 7, 2013

Meetup with Scala IDEA plugin team leader


It was sad to see that JetBrains team leader use Windows for development:

How to create Checkstyle analog for Scala in IDEA IDE:

video will be there: https://thenewcircle.com/s