Add strict formatter [ECR-4138] #1377

dmitry-timofeev · 2020-01-23T11:00:04Z

Overview

Review c4eef90 for changes in configuration and Contributing guide.
edbf7d7 shows what it does:
- Some builder/stream chains are compressed in a single line:
  .setPayload(ExonumMessage.newBuilder().setAnyTx(anyTx(payload)).build().toByteString()) is too much.
- Some artifacts in array initializers.
Subsequent commits fix some artifacts.

See: https://jira.bf.local/browse/ECR-4138

Definition of Done

There are no TODOs left in the code
Change is covered by automated tests
The coding guidelines are followed
Public API has Javadoc
Method preconditions are checked and documented in the Javadoc of the method
Changelog is updated if needed (in case of notable or breaking changes)
The continuous integration build passes

Added a plugin that could reformat all the code automatically to match the Google Java Code Style: https://google.github.io/styleguide/javaguide.html

dmitry-timofeev · 2020-01-23T12:28:16Z

exonum-java-binding/common/src/test/java/com/exonum/binding/common/hash/HashCodeTest.java

+ new byte[] {
+ (byte) 0xef,
+ (byte) 0xcd,
+ (byte) 0xab,
+ (byte) 0x89,
+ (byte) 0x67,
+ (byte) 0x45,
+ (byte) 0x23,
+ (byte) 0x01, // up to here, same bytes as above
+ (byte) 0x01,
+ (byte) 0x02,
+ (byte) 0x03,
+ (byte) 0x04,
+ (byte) 0x05,


Another unfortunate artifactj.

bullet-tooth · 2020-01-23T12:45:34Z

CONTRIBUTING.md

 Java code must follow the [Google code style](https://google.github.io/styleguide/javaguide.html).
-[Checkstyle](http://checkstyle.sourceforge.net/index.html) checks the project 
-during `validate` phase (i.e., _before_ compilation) of the build. You can also run it manually:
+[The format plugin](https://github.com/coveooss/fmt-maven-plugin) checks the project 


hope it works well with IDEA's code format feature 🙃

bullet-tooth · 2020-01-23T12:46:46Z

exonum-java-binding/app/src/main/java/com/exonum/binding/app/ServiceRuntimeBootstrap.java

- version, revision, buildTime);
+ ZonedDateTime buildTime =
+ ZonedDateTime.ofInstant(Instant.ofEpochMilli(timestamp), ZoneOffset.UTC);
+ logger.info(


bullet-tooth · 2020-01-23T12:47:29Z

exonum-java-binding/common/src/main/java/com/exonum/binding/common/blockchain/CallInBlocks.java

 /**
- * Provides factory methods to concisely create
- * {@link com.exonum.core.messages.Blockchain.CallInBlock}s.
+ * Provides factory methods to concisely create {@link


questionable improvement

bullet-tooth · 2020-01-23T12:48:25Z

exonum-java-binding/common/src/main/java/com/exonum/binding/common/blockchain/CallInBlocks.java

 * @param txPosition a zero-based position of the transaction in the block
- * @throws IndexOutOfBoundsException if position is negative or greater than
- * {@value Integer#MAX_VALUE}
+ * @throws IndexOutOfBoundsException if position is negative or greater than {@value


questionable improvement

bullet-tooth · 2020-01-23T12:49:29Z

...ava-binding/common/src/main/java/com/exonum/binding/common/blockchain/ExecutionStatuses.java

- public static final ExecutionStatus SUCCESS = ExecutionStatus.newBuilder()
- .setOk(Empty.getDefaultInstance())
- .build();
+ /** A successful execution status. */


bullet-tooth · 2020-01-23T12:49:53Z

...a-binding/common/src/main/java/com/exonum/binding/common/blockchain/TransactionLocation.java

 public static TypeAdapter<TransactionLocation> typeAdapter(Gson gson) {
 return new AutoValue_TransactionLocation.GsonTypeAdapter(gson);
 }
-


bullet-tooth · 2020-01-23T12:53:59Z

exonum-java-binding/common/src/main/java/com/exonum/binding/common/collect/MapEntry.java

- * <p>A map entry contains <em>a copy</em> of the data in the corresponding map index.
- * Unlike {@link java.util.Map.Entry}, it does not reflect the changes made to the map
- * since this entry had been created.
+ * <p>A map entry contains <em>a copy</em> of the data in the corresponding map index. Unlike {@link


IMO: code reading experience becomes worse here

bullet-tooth · 2020-01-23T12:57:53Z

...ava-binding/common/src/main/java/com/exonum/binding/common/crypto/Ed25519CryptoFunction.java

 public KeyPair generateKeyPair(byte[] seed) {
- checkArgument(hasLength(seed, SEED_BYTES),
- "Seed byte array has invalid size (%s), must be %s", seed.length, SEED_BYTES);
+ checkArgument(


bullet-tooth · 2020-01-23T12:58:17Z

...ava-binding/common/src/main/java/com/exonum/binding/common/crypto/Ed25519CryptoFunction.java

 public boolean verify(byte[] message, byte[] signature, PublicKey publicKey) {
- checkArgument(hasLength(publicKey.toBytesNoCopy(), PUBLIC_KEY_BYTES),
- "Public key has invalid size (%s), must be %s", publicKey.size(), PUBLIC_KEY_BYTES);
+ checkArgument(


bullet-tooth · 2020-01-23T13:11:04Z

exonum-java-binding/common/src/main/java/com/exonum/binding/common/crypto/KeyPair.java

 /**
- * Creates a {@code KeyPair} from two byte arrays, representing {@code privateKey}
- * and {@code publicKey}. All arrays are defensively copied.
+ * Creates a {@code KeyPair} from two byte arrays, representing {@code privateKey} and {@code


don't like such splitting tags

bullet-tooth · 2020-01-23T16:01:09Z

overall looks ok. There are the following major points to notice (to summarize):

One-line chains in builders, streams, etc. Will be difficult to analyze traces in case of exceptions
checkArguments statements become more complex, new param at new line

dmitry-timofeev · 2020-01-23T16:07:17Z

Will be difficult to analyze traces in case of exceptions

I'd add that it sometimes hurts readability, e.g.,

 HashCode valueHash = HASH_FUNCTION.newHasher().putByte(BLOB_PREFIX).putBytes(value.toByteArray()).hash();

or

 Stream<String> packageDirs = artifactClasses.stream().map(Class::getPackage).distinct().map(this::getPath);

dmitry-timofeev added 3 commits January 23, 2020 12:30

Add plugin reformatting the code [ECR-4138].

c4eef90

Added a plugin that could reformat all the code automatically to match the Google Java Code Style: https://google.github.io/styleguide/javaguide.html

Reformat the sources.

edbf7d7

Fix reformatting artifacts

8a53cf6

dmitry-timofeev requested a review from bullet-tooth January 23, 2020 11:00

dmitry-timofeev commented Jan 23, 2020

View reviewed changes

bullet-tooth reviewed Jan 23, 2020

View reviewed changes

dmitry-timofeev added the experimental ⚖️ Experimental PRs usually shall not be merged — they show some approach to get feedback from the team label Jan 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add strict formatter [ECR-4138] #1377

Add strict formatter [ECR-4138] #1377

Uh oh!

dmitry-timofeev commented Jan 23, 2020

dmitry-timofeev Jan 23, 2020

bullet-tooth Jan 23, 2020

bullet-tooth Jan 23, 2020

bullet-tooth Jan 23, 2020

bullet-tooth Jan 23, 2020

bullet-tooth Jan 23, 2020

bullet-tooth Jan 23, 2020

bullet-tooth Jan 23, 2020

bullet-tooth Jan 23, 2020

bullet-tooth Jan 23, 2020

bullet-tooth Jan 23, 2020

bullet-tooth commented Jan 23, 2020

dmitry-timofeev commented Jan 23, 2020

Labels

3 participants

Add strict formatter [ECR-4138] #1377

Are you sure you want to change the base?

Add strict formatter [ECR-4138] #1377

Uh oh!

Conversation

dmitry-timofeev commented Jan 23, 2020

Overview

Definition of Done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bullet-tooth commented Jan 23, 2020

dmitry-timofeev commented Jan 23, 2020

Labels

3 participants