Skip to content

Conversation

rg911
Copy link
Contributor

@rg911 rg911 commented Dec 18, 2019

Fixed #390

@fboucquez
Copy link
Contributor

fboucquez commented Dec 19, 2019

Hi @rg911 , I'm sorry for the late review.

I think the assign operation is a bit complicated and verbose

Object.assign({__proto__: Object.getPrototypeOf(someObject)}, someObject, anotherObject)

What I would do is hide that into a static helper method (DtoMapping.ts maybe) and call that method where you need to do the cloning and copy ops.

For example:

 /**
     * This method creates a copy of the first object adding the attributes of the second object.
     * @param object the object to be cloned
     * @param attributes the extra attributes to be added to the object.
     * @returns a copy of the first object with the new attributes added.
     */
    public static assign<T>(object: T, attributes: any): T {
        return Object.assign({__proto__: Object.getPrototypeOf(object)}, object, attributes);
    }

and change the callers like:

 /**
     * Set transaction maxFee using fee multiplier
     * @param feeMultiplier The fee multiplier
     * @returns {TransferTransaction}
     */
    public setMaxFee(feeMultiplier: number): Transaction {
        return DtoMapping.assign(this, {maxFee: UInt64.fromUint(this.size * feeMultiplier)});
    }

Benefits:

  1. Easier to understand.
  2. Quite used operation
  3. If there is a better way (or not buggy, sorry for the spread operation suggestion I missed that ts secondary effect) and you need to change it, you would change it in one place only.

What do you think?

@rg911
Copy link
Contributor Author

rg911 commented Dec 22, 2019

Hi @rg911 , I'm sorry for the late review.

I think the assign operation is a bit complicated and verbose

Object.assign({__proto__: Object.getPrototypeOf(someObject)}, someObject, anotherObject)

What I would do is hide that into a static helper method (DtoMapping.ts maybe) and call that method where you need to do the cloning and copy ops.

For example:

 /**
     * This method creates a copy of the first object adding the attributes of the second object.
     * @param object the object to be cloned
     * @param attributes the extra attributes to be added to the object.
     * @returns a copy of the first object with the new attributes added.
     */
    public static assign<T>(object: T, attributes: any): T {
        return Object.assign({__proto__: Object.getPrototypeOf(object)}, object, attributes);
    }

and change the callers like:

 /**
     * Set transaction maxFee using fee multiplier
     * @param feeMultiplier The fee multiplier
     * @returns {TransferTransaction}
     */
    public setMaxFee(feeMultiplier: number): Transaction {
        return DtoMapping.assign(this, {maxFee: UInt64.fromUint(this.size * feeMultiplier)});
    }

Benefits:

  1. Easier to understand.
  2. Quite used operation
  3. If there is a better way (or not buggy, sorry for the spread operation suggestion I missed that ts secondary effect) and you need to change it, you would change it in one place only.

What do you think?

Fixed

Comment on lines +69 to 72
repositoryFactory.getGenerationHash().subscribe((gh) => {
expect(counter).to.be.equals(1);
expect(gh).to.be.equals('aaaa');
repositoryFactory.getGenerationHash().subscribe(gh => {
Copy link
Contributor

@fboucquez fboucquez Dec 23, 2019

Choose a reason for hiding this comment

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

Thanks, @rg911 for the cleanup. Question, why the braces in arrow function's parameter when it is not necessary? Do you prefer that code style? Is it lint?

@rg911 rg911 merged commit 7867c9f into symbol:master Dec 23, 2019
@fboucquez fboucquez deleted the task/g390_max_fee branch December 23, 2019 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Transaction#setMaxFee() returns a broken transaction.

2 participants