Skip to content

Commit

Permalink
Code style refactoring
Browse files Browse the repository at this point in the history
  • Loading branch information
Artem Labazin authored and Artem Labazin committed Mar 16, 2018
1 parent a8241f9 commit 965364b
Show file tree
Hide file tree
Showing 25 changed files with 106 additions and 202 deletions.
14 changes: 3 additions & 11 deletions .codestyle/checkstyle.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,6 @@ limitations under the License.
</module>


<!--http://checkstyle.sourceforge.net/config_sizes.html#FileLength-->
<module name="FileLength">
<property name="max" value="300"/>
</module>


<!--http://checkstyle.sourceforge.net/config_whitespace.html#FileTabCharacter-->
<module name="FileTabCharacter">
<property name="eachLine" value="true"/>
Expand All @@ -69,8 +63,11 @@ limitations under the License.
<module name="Translation"/>
<module name="UniqueProperties"/>

<module name="SuppressWarningsFilter" />


<module name="TreeWalker">
<module name="SuppressWarningsHolder" />

<!-- Checks for Javadoc comments. -->
<!-- See http://checkstyle.sf.net/config_javadoc.html -->
Expand Down Expand Up @@ -318,9 +315,6 @@ limitations under the License.
<!-- Checks for common coding problems -->
<!-- See http://checkstyle.sf.net/config_coding.html -->
<module name="CovariantEquals"/>
<module name="DeclarationOrder">
<property name="ignoreConstructors" value="true"/>
</module>
<module name="DefaultComesLast"/>
<module name="EmptyStatement"/>
<module name="EqualsAvoidNull"/>
Expand Down Expand Up @@ -359,7 +353,6 @@ limitations under the License.
<module name="SuperClone"/>
<module name="SuperFinalize"/>
<module name="UnnecessaryParentheses"/>
<module name="VariableDeclarationUsageDistance"/>


<!-- Checks for class design -->
Expand All @@ -369,7 +362,6 @@ limitations under the License.
</module>
<module name="FinalClass"/>
<module name="HideUtilityClassConstructor"/>
<module name="MutableException"/>
<module name="OneTopLevelClass"/>

<!-- Miscellaneous other checks. -->
Expand Down
3 changes: 3 additions & 0 deletions .codestyle/findbugs.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,7 @@ limitations under the License.
<Match>
<Bug pattern="RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE"/>
</Match>
<Match>
<Bug pattern="BC_UNCONFIRMED_CAST"/>
</Match>
</FindBugsFilter>
167 changes: 41 additions & 126 deletions .codestyle/pmd.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,144 +23,59 @@ limitations under the License.

<description>PMD Rules</description>

<rule ref="rulesets/java/basic.xml">
<rule ref="category/java/bestpractices.xml">
<exclude name="AvoidUsingHardCodedIP" />
<exclude name="UseVarargs" />
</rule>

<rule ref="rulesets/java/braces.xml" />
<rule ref="rulesets/java/clone.xml" />
<rule ref="rulesets/java/unusedcode.xml"/>
<rule ref="rulesets/java/finalizers.xml" />


<rule ref="rulesets/java/imports.xml">
<!-- Stupid rule... -->
<rule ref="category/java/codestyle.xml">
<exclude name="AbstractNaming" />
<exclude name="AtLeastOneConstructor" />
<exclude name="AvoidPrefixingMethodParameters" />
<exclude name="CommentDefaultAccessModifier" />
<exclude name="DefaultPackage" />
<exclude name="FieldDeclarationsShouldBeAtStartOfClass" />
<exclude name="LocalVariableCouldBeFinal" />
<exclude name="LongVariable" />
<exclude name="MethodArgumentCouldBeFinal" />
<exclude name="OnlyOneReturn" />
<exclude name="ShortClassName" />
<exclude name="ShortMethodName" />
<exclude name="ShortVariable" />
<exclude name="TooManyStaticImports" />
<exclude name="UselessParentheses" />
</rule>

<rule ref="rulesets/java/codesize.xml" >
<!-- See the explanation for TooManyMethods.-->
<rule ref="category/java/design.xml">
<exclude name="AvoidCatchingGenericException" />
<exclude name="AvoidRethrowingException" />
<exclude name="AvoidThrowingNullPointerException" />
<exclude name="DataClass" />
<exclude name="ExcessiveImports" />
<exclude name="ExcessivePublicCount" />
<exclude name="LawOfDemeter" />
<exclude name="LoosePackageCoupling" />
<exclude name="TooManyFields" />
<!-- Design is very hard to measure by numbers like this.-->
<!-- The number and quality of dependencies might be a better indicator, -->
<!-- and that requires a different tool.-->
<exclude name="TooManyMethods" />
<!-- See the explanation for TooManyMethods.-->
<exclude name="ExcessivePublicCount" />
<!-- Needs better understanding and proper configuration-->
<exclude name="StdCyclomaticComplexity" />
<!-- Needs better understanding and proper configuration-->
<exclude name="ModifiedCyclomaticComplexity" />
<!-- See the explanation for TooManyMethods.-->
<exclude name="ExcessiveParameterList" />
<!-- Too abstract rule -->
<exclude name="CyclomaticComplexity" />
</rule>

<rule ref="rulesets/java/design.xml">
<!-- Sometimes important to avoid "DOS attack" but not as a general rule-->
<exclude name="AvoidSynchronizedAtMethodLevel" />
<!-- It's just extra effort to write and read the final keyword-->
<exclude name="ClassWithOnlyPrivateConstructorsShouldBeFinal" />
<!-- Maybe good idea if PMD could exclude null checks from this-->
<exclude name="ConfusingTernary" />
<!-- Statistical analysis is prone to givin false positives. Potential god classes-->
<!-- most probably violate something else, too.-->
<!-- And dependency analysis tools also help here.-->
<exclude name="GodClass" />
<!-- Switch is sometimes very readable-->
<exclude name="TooFewBranchesForASwitchStatement"/>
<!-- A static utility is a static utility even if it masquerades as something-->
<!-- else by using the Singleton pattern-->
<exclude name="UseUtilityClass" />
<!-- This is good advice, but since it's violated so much in this project-->
<!-- and the problem is not big (especially with good syntax colouring),-->
<!-- we'll keep this ignored for now.-->
<exclude name="AvoidReassigningParameters" />
<!-- Good idea almost always, but not quite.-->
<!--<exclude name="ReturnEmptyArrayRatherThanNull" />-->
<!-- Sometimes one step at a time makes clearer code.-->
<!-- Debugging is also easier if the return value is in a variable.-->
<exclude name="UnnecessaryLocalBeforeReturn" />
<!-- There are valid reasons for passing arrays (making it nullable for example)-->
<exclude name="UseVarargs" />
<!-- TODO explain what false positives this gives-->
<exclude name="MissingBreakInSwitch" />
<!-- TODO enable when all findings have been fixed-->
<exclude name="UseLocaleWithCaseConversions" />
<!-- It gives a lot of warnings on 'equals' method, fixing would decrease readability-->
<exclude name="SimplifyBooleanReturns"/>
<!--Gives false positive-->
<exclude name="FieldDeclarationsShouldBeAtStartOfClass"/>
<!-- Good rule but in practice to often suppressed -->
<exclude name="PreserveStackTrace"/>

<exclude name="AccessorMethodGeneration"/>
<!-- remove it -->
<exclude name="NcssCount" />
</rule>
<rule ref="category/java/documentation.xml">
<exclude name="CommentSize" />

<rule ref="rulesets/java/migrating.xml">
<!-- The annotation is not as readable and there is no way to state which-->
<!-- line should throw the exception and with what message-->
<exclude name="JUnitUseExpected" />
<!-- Main code is not junit code-->
<exclude name="JUnit4TestShouldUseTestAnnotation" />
<!-- remove it -->
<exclude name="CommentRequired" />
</rule>

<rule ref="rulesets/java/naming.xml">
<!-- Often good to start name with Abstract, but on the other hand this-->
<!-- rule is a bit too much like Hungarian notation and I in interface names.-->
<!-- Who cares if it's abstract or not when you are using it?-->
<exclude name="AbstractNaming" />
<!-- Opinion, for me a getter is not a command, it's a declarative-->
<!-- data reference-->
<rule ref="category/java/errorprone.xml">
<exclude name="AvoidFieldNameMatchingMethodName" />
<!-- Why should generics not be named properly, like all other things-->
<!-- (well, except Windows filesystem roots)?-->
<exclude name="GenericsNaming" />
<!-- It can be long if it's the only way to make it good-->
<exclude name="LongVariable" />
<!-- It can be short if it's good-->
<exclude name="ShortVariable" />
<!-- TODO explain why.-->
<exclude name="BooleanGetMethodName" />
<!-- It can be short if it's good-->
<exclude name="ShortClassName" />
<!-- It can be short if it's good-->
<exclude name="ShortMethodName" />
</rule>

<rule ref="rulesets/java/optimizations.xml">
<!-- Too many false hits. Optimization can't be done with static analysis.
Besides, following this may encourage the antipattern of using too broad
scope for stuff: -->
<exclude name="AvoidInstantiatingObjectsInLoops" />
<!-- Good principle but too verbose in practice: -->
<exclude name="MethodArgumentCouldBeFinal" />
<!-- Good principle and maybe sometimes even practical but not in this
project: -->
<exclude name="LocalVariableCouldBeFinal" />
<exclude name="RedundantFieldInitializer" />
<exclude name="AvoidFieldNameMatchingTypeName" />
<exclude name="BeanMembersShouldSerialize" />
<exclude name="DataflowAnomalyAnalysis" />
<exclude name="DoNotCallSystemExit" />
<exclude name="MissingBreakInSwitch" />
</rule>

<rule ref="rulesets/java/strictexception.xml" >
<!-- NPE communicates very cleary what is happening, it is not-->
<!-- interesting who reports it (jvm or user code)-->
<exclude name="AvoidThrowingNullPointerException" />
<!-- TODO explain why-->
<exclude name="AvoidCatchingGenericException" />
<!-- TODO explain why-->
<exclude name="AvoidThrowingRawExceptionTypes" />
<!-- One valid case is to catch runtime, throw as such and after that-->
<!-- catch Exception and wrap as runtime.-->
<!-- Without the first all runtimes are unnecessarily wrapped.-->
<exclude name="AvoidRethrowingException" />
<!-- There are case which should throws generic exceptions -->
<exclude name="SignatureDeclareThrowsException" />
<rule ref="category/java/multithreading.xml">
</rule>

<rule ref="rulesets/java/strings.xml" >
<!-- Splitting to multiple lines is sometimes more readable.-->
<!-- Besides, where's the proof that it affects performance?-->
<exclude name="ConsecutiveAppendsShouldReuse" />
<rule ref="category/java/performance.xml">
<exclude name="AvoidUsingShortType" />
</rule>
</ruleset>
9 changes: 9 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,15 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

- Add more unit and integration tests.

## [1.0.0](https://github.com/appulse-projects/epmd-java/releases/tag/1.0.0) - 2018-03-16

Code style refactoring.

### Changed

- PMD, FindBugs and Checkstyle fixes;
- Updated dependencies.

## [0.4.2](https://github.com/appulse-projects/epmd-java/releases/tag/0.4.2) - 2018-03-05

Removed lookup cache, which was full of bugs
Expand Down
4 changes: 2 additions & 2 deletions client/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ Include the dependency to your project's pom.xml file:
<dependency>
<groupId>io.appulse.epmd.java</groupId>
<artifactId>client</artifactId>
<version>0.4.2</version>
<version>1.0.0</version>
</dependency>
...
</dependencies>
Expand All @@ -23,7 +23,7 @@ Include the dependency to your project's pom.xml file:
or Gradle:

```groovy
compile 'io.appulse.epmd.java:client:0.4.2'
compile 'io.appulse.epmd.java:client:1.0.0'
```

### Create client
Expand Down
2 changes: 1 addition & 1 deletion client/pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ limitations under the License.
<parent>
<groupId>io.appulse</groupId>
<artifactId>epmd-java</artifactId>
<version>0.4.2</version>
<version>1.0.0</version>
</parent>

<groupId>io.appulse.epmd.java</groupId>
Expand Down
2 changes: 1 addition & 1 deletion client/src/main/java/io/appulse/epmd/java/client/EpmdClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ private static int getDefaultPort () {
port = Integer.parseInt(value);
}
} catch (NumberFormatException | SecurityException ex) {
throw new RuntimeException(ex);
throw new IllegalStateException("Couldn't extract EPMD port", ex);
}
log.debug("Default EPMD port is: '{}'", port);
return port;
Expand Down
21 changes: 0 additions & 21 deletions client/src/test/java/io/appulse/epmd/java/client/LocalEpmdClientTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import static lombok.AccessLevel.PRIVATE;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static io.appulse.epmd.java.core.model.response.EpmdDump.NodeDump.Status.OLD_OR_UNUSED;

import io.appulse.epmd.java.client.exception.EpmdConnectionException;
import io.appulse.epmd.java.client.exception.EpmdRegistrationException;
Expand Down Expand Up @@ -189,26 +188,6 @@ public void dumpAll () {
});
}

// @Test
public void stop () {
client.register("stopped", 19028, R3_ERLANG, TCP, R6, R6);

client.stop("stopped");

val nodes = client.dumpAll();
assertThat(nodes)
.isNotEmpty();

val node = nodes.stream()
.filter(it -> "stopped".equals(it.getName()))
.findFirst()
.orElse(null);

assertThat(node)
.isNotNull()
.extracting("status").isEqualTo(OLD_OR_UNUSED);
}

@Test
public void kill () {
assertThat(client.kill())
Expand Down
2 changes: 1 addition & 1 deletion core/pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ limitations under the License.
<parent>
<groupId>io.appulse</groupId>
<artifactId>epmd-java</artifactId>
<version>0.4.2</version>
<version>1.0.0</version>
</parent>

<groupId>io.appulse.epmd.java</groupId>
Expand Down
4 changes: 2 additions & 2 deletions core/src/test/java/io/appulse/epmd/java/core/model/request/RegistrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@

package io.appulse.epmd.java.core.model.request;

import static io.appulse.epmd.java.core.model.Tag.ALIVE2_REQUEST;
import static org.assertj.core.api.Assertions.assertThat;
import static io.appulse.epmd.java.core.model.NodeType.R3_HIDDEN;
import static io.appulse.epmd.java.core.model.Protocol.SCTP;
import static io.appulse.epmd.java.core.model.Tag.ALIVE2_REQUEST;
import static io.appulse.epmd.java.core.model.Version.R6;
import static org.assertj.core.api.Assertions.assertThat;

import io.appulse.epmd.java.core.mapper.deserializer.MessageDeserializer;
import io.appulse.epmd.java.core.mapper.serializer.MessageSerializer;
Expand Down
2 changes: 1 addition & 1 deletion core/src/test/java/io/appulse/epmd/java/core/model/response/EpmdInfoTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
package io.appulse.epmd.java.core.model.response;

import static java.nio.charset.StandardCharsets.ISO_8859_1;
import static org.assertj.core.groups.Tuple.tuple;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.groups.Tuple.tuple;

import io.appulse.epmd.java.core.mapper.deserializer.MessageDeserializer;
import io.appulse.epmd.java.core.mapper.serializer.MessageSerializer;
Expand Down
4 changes: 2 additions & 2 deletions core/src/test/java/io/appulse/epmd/java/core/model/response/NodeInfoTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@
package io.appulse.epmd.java.core.model.response;

import static io.appulse.epmd.java.core.model.NodeType.R4_HIDDEN;
import static java.nio.charset.StandardCharsets.ISO_8859_1;
import static io.appulse.epmd.java.core.model.Protocol.UDP;
import static io.appulse.epmd.java.core.model.Version.R4;
import static io.appulse.epmd.java.core.model.Tag.PORT2_RESPONSE;
import static io.appulse.epmd.java.core.model.Version.R4;
import static java.nio.charset.StandardCharsets.ISO_8859_1;
import static org.assertj.core.api.Assertions.assertThat;

import io.appulse.epmd.java.core.mapper.deserializer.MessageDeserializer;
Expand Down
1 change: 1 addition & 0 deletions core/src/test/java/io/appulse/epmd/java/core/model/response/RegistrationResultTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import io.appulse.epmd.java.core.mapper.deserializer.MessageDeserializer;
import io.appulse.epmd.java.core.mapper.serializer.MessageSerializer;
import io.appulse.utils.Bytes;

import lombok.val;
import org.junit.Test;

Expand Down
2 changes: 1 addition & 1 deletion core/src/test/java/io/appulse/epmd/java/core/model/response/StopResultTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@

package io.appulse.epmd.java.core.model.response;

import static org.assertj.core.api.Assertions.assertThat;
import static java.nio.charset.StandardCharsets.ISO_8859_1;
import static org.assertj.core.api.Assertions.assertThat;

import io.appulse.epmd.java.core.mapper.deserializer.MessageDeserializer;
import io.appulse.epmd.java.core.mapper.serializer.MessageSerializer;
Expand Down
Loading

0 comments on commit 965364b

Please sign in to comment.