Skip to content

Conversation

bullet-tooth
Copy link
Contributor

@bullet-tooth bullet-tooth commented Jan 8, 2020

Overview


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

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

@coveralls
Copy link

coveralls commented Jan 8, 2020

Coverage Status

Coverage decreased (-0.8%) to 85.049% when pulling 3da9f0a on ecr-2746-new into 1a246f9 on master.

* check(amount <= balance, (byte)3,
* "Not enough money. Operation amount is %s, but actual balance was %s",
* amount, balance);
* }</pre>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+ a back link from ExecutionException.

 * @see ExecutionException

* amount, balance);
* }</pre>
*/
public final class Preconditions {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExecutionPreconditions (as they are tied to ExecException)? That will allow
to use both Guava's and ours as simple names.

Also, I'd ponder on the name of 'check' methods. 'check' seems too vague. Shall
we use 'checkExecution' (because Execution Exception, also, it is similar to Guava's naming 'checkArgument' -> Illegal Argument Exception)? Do you have any other names in mind?

*
* <p>See {@link #check(boolean, byte, String, Object...)} for details.
*/
public static void check(boolean expression, byte errorCode,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How the set of overloads was choosen?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A fascinating talk on how they do it at Google, helping design not only APIs but languages: https://www.youtube.com/watch?v=sPW2Pz2dI9E&list=LLXsc0bWTsranC6H8z1IOoXg&index=10&t=0s (@vitvakatu , @slowli )

That's what we often miss — data on the actual usages to make informed decisions, instead of speculations 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just tried to cover most frequently used cases + inspired by guava lib 🙃

* @param expression a boolean expression
* @param errorCode execution error code
* @param errorMessageTemplate execution error description template to use if the check fails.
* The template could have placeholders {@code %s} which will be replaces by arguments
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replaced

`ProofListIndexProxy.getProof`, `ProofListIndexProxy.getRangeProof` and
`ListProof`.
- `ProofEntryIndexProxy` collection.
- Transaction precondition utility methods.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specify class name?

MapIndex<PublicKey, Wallet> wallets = schema.wallets();

checkExecution(!wallets.containsKey(ownerPublicKey), WALLET_ALREADY_EXISTS.errorCode);
ExecutionPreconditions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shan't we use static imports for it?

Copy link
Contributor

@dmitry-timofeev dmitry-timofeev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And QA service, please 🙃

@dmitry-timofeev dmitry-timofeev merged commit 6aabf29 into master Jan 23, 2020
@dmitry-timofeev dmitry-timofeev deleted the ecr-2746-new branch January 23, 2020 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants