Issue 4248 - Add codestyle check

third_party/checkstyle/java/rules/eyeo_checks.xml

Index: third_party/checkstyle/java/rules/eyeo_checks.xml
diff --git a/third_party/checkstyle/java/rules/eyeo_checks.xml b/third_party/checkstyle/java/rules/eyeo_checks.xml
new file mode 100644
index 0000000000000000000000000000000000000000..bfe8c41ea1c7dd54f4cceb3fe7c8e236c3ecff68
--- /dev/null
+++ b/third_party/checkstyle/java/rules/eyeo_checks.xml
@@ -0,0 +1,178 @@
+<?xml version="1.0"?>
+<!DOCTYPE module PUBLIC
+          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
+          "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">
+
+<!--
+
+  Checkstyle configuration that checks the sun coding conventions from:
+
+    - the Java Language Specification at
+      http://java.sun.com/docs/books/jls/second_edition/html/index.html
+
+    - the Sun Code Conventions at http://java.sun.com/docs/codeconv/
+
+    - the Javadoc guidelines at
+      http://java.sun.com/j2se/javadoc/writingdoccomments/index.html
+
+    - the JDK Api documentation http://java.sun.com/j2se/docs/api/index.html
+
+    - some best practices
+
+  Checkstyle is very configurable. Be sure to read the documentation at
+  http://checkstyle.sf.net (or in your downloaded distribution).
+
+  Most Checks are configurable, be sure to consult the documentation.
+
+  To completely disable a check, just comment it out or delete it from the file.
+
+  Finally, it is worth reading the documentation.
+
+-->
+
+<module name="Checker">
+    <!--
+        If you set the basedir property below, then all reported file
+        names will be relative to the specified directory. See
+        http://checkstyle.sourceforge.net/5.x/config.html#Checker
+
+        <property name="basedir" value="${basedir}"/>
+    -->
+
+    <property name="fileExtensions" value="java, properties, xml"/>
+
+    <!-- Checks whether files end with a new line.                        -->
+    <!-- See http://checkstyle.sf.net/config_misc.html#NewlineAtEndOfFile -->
+    <module name="NewlineAtEndOfFile"/>

[diegocarloslima, 2016/11/03 14:00:51]

I think that it would be good to organize our file by grouping all modules
declarations that belongs to the same section in checkstyle.sf.net
documentation. For instance, this module should be together with all other
modules that belongs to the misc category
(http://checkstyle.sourceforge.net/config_misc.html)

[anton, 2016/11/07 07:40:44]

No problem

+
+    <!-- Checks for Size Violations.                    -->
+    <!-- See http://checkstyle.sf.net/config_sizes.html -->
+    <module name="FileLength"/>
+
+    <!-- Checks for whitespace                               -->
+    <!-- See http://checkstyle.sf.net/config_whitespace.html -->
+    <module name="FileTabCharacter"/>
+
+    <!-- Miscellaneous other checks.                   -->
+    <!-- See http://checkstyle.sf.net/config_misc.html -->
+    <module name="RegexpSingleline">
+        <property name="format" value="\s+$"/>
+        <property name="minimum" value="0"/>
+        <property name="maximum" value="0"/>
+        <property name="message" value="Line has trailing spaces."/>
+    </module>
+
+    <!-- Checks for Headers                                -->
+    <!-- See http://checkstyle.sf.net/config_header.html   -->
+    <module name="Header">

[anton, 2016/07/19 06:45:44]

added this rule (pass filename as argument) since we are required to add it in
each java file

+        <property name="headerFile" value="${checkstyle.header.file}"/>
+        <property name="fileExtensions" value="java"/>
+    </module>
+
+    <module name="TreeWalker">
+
+        <!-- Checks for Javadoc comments.                     -->
+        <!-- See http://checkstyle.sf.net/config_javadoc.html -->
+        <!-- <module name="JavadocMethod"/> -->

[anton, 2016/07/19 06:45:44]

i know i need to remove commented out lines, but left here just to illustrate
it's available.

+        <!-- <module name="JavadocType"/> -->
+        <!-- <module name="JavadocVariable"/> -->
+        <!-- <module name="JavadocStyle"/> -->
+
+        <!-- Checks for Naming Conventions.                  -->
+        <!-- See http://checkstyle.sf.net/config_naming.html -->
+        <module name="ConstantName"/>
+        <module name="LocalFinalVariableName"/>
+        <module name="LocalVariableName"/>
+        <module name="MemberName"/>
+        <module name="MethodName"/>
+        <module name="PackageName"/>
+        <module name="ParameterName"/>
+        <module name="StaticVariableName"/>
+        <module name="TypeName"/>
+
+        <!-- Checks for imports                              -->
+        <!-- See http://checkstyle.sf.net/config_import.html -->

[diegocarloslima, 2016/11/03 14:00:51]

This link is broken, it should be replaced by
http://checkstyle.sourceforge.net/config_imports.html

[anton, 2016/11/07 07:40:44]

Acknowledged.

+        <module name="AvoidStarImport"/>
+        <module name="IllegalImport"/> <!-- defaults to sun.* packages -->
+        <module name="RedundantImport"/>
+        <module name="UnusedImports">
+            <property name="processJavadoc" value="false"/>
+        </module>
+        <module name="ImportOrder">
+            <!-- Android's import guidelines: http://source.android.com/source/code-style.html#order-import-statements -->
+            <property name="groups" value="org.mozilla,com,net,org,android,java,javax"/>

[anton, 2016/07/19 06:45:44]

according to mozilla's style org.mozilla should be the first? do we need to have
the same rule?

[diegocarloslima, 2016/11/03 14:00:51]

We usually keep like that for files that are maintained by Mozilla, but I don't
think that it applies to our files

[René Jeschke, 2016/11/24 14:28:18]

As this is for having 'in-house' imports at the top, we can remove the
'org.mozilla' part. AFAIK we do not have rules for our imports, but if we want
to go the Mozilla way here, then we could replace 'org.mozilla' with
'org.adblockplus'.

[anton, 2016/12/02 06:21:08]

Acknowledged.

+            <property name="separated" value="true"/>
+        </module>
+
+        <!-- Checks for Size Violations.                    -->
+        <!-- See http://checkstyle.sf.net/config_sizes.html -->
+        <module name="LineLength">
+            <property name="max" value="100"/>

[anton, 2016/07/21 10:12:47]

according to mozilla's style one could have lines a bit longer than 100 chars if
wrapping hurts readability. Using 100 for now

+        </module>
+        <module name="MethodLength"/>
+        <module name="ParameterNumber"/>
+
+        <!-- Checks for whitespace                               -->
+        <!-- See http://checkstyle.sf.net/config_whitespace.html -->
+        <module name="EmptyForIteratorPad"/>
+        <module name="GenericWhitespace"/>
+        <module name="MethodParamPad"/>
+        <module name="NoWhitespaceAfter"/>
+        <module name="NoWhitespaceBefore"/>
+        <!-- <module name="OperatorWrap"/> -->
+        <module name="ParenPad"/>
+        <module name="TypecastParenPad"/>
+        <module name="WhitespaceAfter"/>

[anton, 2016/07/21 10:12:47]

could be changed from default to COMMA and SEMI only:
http://checkstyle.sourceforge.net/config_whitespace.html#WhitespaceAfter

Otherwise we will have error after (int) typecast in Filter.java f.e.:
@Override
  public int hashCode()
  {
    return (int)(this.ptr >> 32) * (int)this.ptr;
  }

[diegocarloslima, 2016/11/03 14:00:50]

I personally prefer a white space after a typecast, but no strong opinion about
that

[René Jeschke, 2016/11/21 11:44:20]

I think we have spaces after type casts ... but I'm not 100% sure.

+        <module name="WhitespaceAround"/>
+
+        <!-- Modifier Checks                                    -->
+        <!-- See http://checkstyle.sf.net/config_modifiers.html -->
+        <module name="ModifierOrder"/>

[anton, 2016/07/21 10:12:46]

currently we have "final static" almost in most places (f.e.), but "static
final" is recommended by ModifierOrder:
http://checkstyle.sourceforge.net/config_modifier.html

totally up to us to decide what code style we support.

[diegocarloslima, 2016/11/03 14:00:51]

I would go for the recommended one, 'static final'

[anton, 2016/11/07 07:40:44]

Agree, i prefer "static final" too. Rene?

[René Jeschke, 2016/11/21 11:44:19]

Yes, I also strongly prefer using the recommended way here, so it should be
'static final'.

[anton, 2016/11/23 12:01:11]

Acknowledged.

+        <module name="RedundantModifier"/>
+
+        <!-- Checks for blocks. You know, those {}'s         -->
+        <!-- See http://checkstyle.sf.net/config_blocks.html -->
+        <module name="AvoidNestedBlocks"/>
+        <module name="EmptyBlock"/>
+        <module name="LeftCurly">
+            <property name="option" value="nl"/>

[anton, 2016/07/19 06:45:45]

according to eyeo's style on new line, f.e.

void func()
{
...

BTW, what is the reason to skip traditional java-style like

void func() {
...

[René Jeschke, 2016/11/21 11:44:20]

Well, I could list reasons why having the curlies on the next line is better
than on the same line, but it doesn't matter in the end. It is like it is, and I
love it. ^^

[anton, 2016/11/23 12:01:12]

No problem.

+        </module>
+        <module name="NeedBraces"/>
+        <module name="RightCurly">
+            <property name="option" value="alone_or_singleline"/>
+        </module>
+
+        <!-- Checks for common coding problems               -->
+        <!-- See http://checkstyle.sf.net/config_coding.html -->
+        <module name="AvoidInlineConditionals"/>

[anton, 2016/07/19 06:45:44]

is it bad to use (case?positive_branch:negative_branch)?
http://checkstyle.sourceforge.net/config_coding.html#AvoidInlineConditionals

actually i do use it time to time

[diegocarloslima, 2016/11/03 14:00:50]

I also use it from time to time. I think it depends on how big the inline
conditional is. If is too big with many operands, it might hurt the readability

[anton, 2016/11/07 07:40:44]

Agree. I think we need one person (like Rene or Felix) to take the solution as
it's a matter of tastes IMHO.

[René Jeschke, 2016/11/21 11:44:19]

I don't think we should disable the ternary operator. There are various
situations where this operator makes things a lot more readable instead of
having 9 lines of code (if/else).

Also, this goes nicely with 'final' variables, makes more sense than using
if/else.

[anton, 2016/11/23 12:01:12]

Acknowledged.

+        <module name="EmptyStatement"/>
+        <module name="EqualsHashCode"/>
+        <!-- <module name="HiddenField"/> -->
+        <module name="IllegalInstantiation"/>
+        <module name="InnerAssignment"/>
+        <module name="MagicNumber"/>

[anton, 2016/07/21 10:12:46]

a bit discussed it with Rene. In tests we can have this (f.e.
Thread.sleep(1000)), but it seems to be okay. I believe it's better to have some
comment about why 1000 is appropriate here or why we should wait for such time.
Anyway this is not allowed in production code but seems to be reasonable for
tests. After brief discussion we decided to have the same rules set for both
production code and test code and have strict rules for tests too. To be
discussed more if/when needed.

+        <module name="MissingSwitchDefault"/>
+        <module name="SimplifyBooleanExpression"/>
+        <module name="SimplifyBooleanReturn"/>
+
+        <!-- Checks for class design                         -->
+        <!-- See http://checkstyle.sf.net/config_design.html -->
+        <!-- <module name="DesignForExtension"/> -->
+        <module name="FinalClass"/>
+        <module name="HideUtilityClassConstructor"/>
+        <module name="InterfaceIsType"/>
+        <!-- <module name="VisibilityModifier"/> -->
+
+        <!-- Miscellaneous other checks.                   -->
+        <!-- See http://checkstyle.sf.net/config_misc.html -->
+        <module name="ArrayTypeStyle"/>
+        <module name="Indentation">
+            <property name="basicOffset" value="2"/>
+        </module> 
+        <!-- <module name="FinalParameters"/> -->
+        <module name="TodoComment"/>

[anton, 2016/07/21 10:12:47]

afaik it's not allowed to add TODO: but actually we have some places with TODOs.
I believe we should have this rule and fix this in the code

[René Jeschke, 2016/11/21 11:44:20]

Yes, we should not use TODO/FIXME/XXX comments in code and use the issue tracker
instead.

[anton, 2016/11/23 12:01:11]

Acknowledged.

+        <module name="UpperEll"/>

[anton, 2016/07/19 06:45:44]

does it make sense to write L instead of l? f.e. 10L vs. 10l

[diegocarloslima, 2016/11/03 14:00:51]

I think it does, helps to maintain good readability and avoid misleadings

[anton, 2016/11/07 07:40:44]

Usually it's highlighted by IDE so it's not a problem until you write in
notepad. However i would agree L is slightly better for readability

[René Jeschke, 2016/11/21 11:44:20]

Yes, 'L' is a lot better readable than 'l'.

[anton, 2016/11/23 12:01:11]

Acknowledged.

+
+    </module>
+
+</module>