Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[fix] correct dependencies and [refactor] rewrite app.js using react hooks and [chore] clean up #2

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

[fix] correct dependencies and [refactor] rewrite app.js using react hooks and [chore] clean up #2

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6aa45da
[fix] upgrade truffle version
anton-karlovskiy Feb 10, 2021
7174f56
[chore] clean up
anton-karlovskiy Feb 10, 2021
668fbd6
[refactor] rewrite app.js using react hooks
anton-karlovskiy Feb 10, 2021
eccfe25
[chore] clean up
anton-karlovskiy Feb 10, 2021
82831c0
[chore] add contributors
anton-karlovskiy Feb 10, 2021
0f0eadb
fix: avoid redeclaring color variable in the loop
anton-karlovskiy Mar 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions test/Color.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,15 @@ contract('Color', (accounts) => {
it('creates a new token', async () => {
const result = await contract.mint('#EC058E');
const totalSupply = await contract.totalSupply();
// SUCCESS

// Success
assert.equal(totalSupply, 1);
const event = result.logs[0].args;
assert.equal(event.tokenId.toNumber(), 1, 'id is correct');
assert.equal(event.from, '0x0000000000000000000000000000000000000000', 'from is correct');
assert.equal(event.to, accounts[0], 'to is correct');

// FAILURE: cannot mint same color twice
// Failure: cannot mint same color twice
await contract.mint('#EC058E').should.be.rejected;
});
});
Expand All @@ -56,11 +57,10 @@ contract('Color', (accounts) => {
await contract.mint('#000000');
const totalSupply = await contract.totalSupply();

let color;
let result = [];

for (let index = 0; index < totalSupply; index++) {
color = await contract.colors(index);
const color = await contract.colors(index);
Copy link

Choose a reason for hiding this comment

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

Why are you redeclaring the const in each loop run?

Copy link
Author

Choose a reason for hiding this comment

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

Actually, there was no declaration of color variable.
Now I've fixed like so:

let color;
for (let index = 0; index < totalSupply; index++) {
  color = await contract.colors(index);
  result.push(color);
};

Copy link

@KinectTheUnknown KinectTheUnknown Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Why are you redeclaring the const in each loop run?

It doesn't redeclare since const is block-scoped. Therefore, each iteration is completely isolated (in terms of block-scope). const inside the for loop ensures (and make it clear that) the variable is not being reassigned during any given iteration and keeps the variable only where it is needed/used.

You can try out the following snippet for proof of concept.

const array = ["foo", "bar", "baz"];
for (let i = 0; i < array.length; i++) {
  const item = array[i];
  console.log(item);
}

It should log each item in the array and not throw an error about item already being declared (because each iteration is isolated by block-scope).

Copy link

Choose a reason for hiding this comment

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

Ah, thanks. I was not aware of that. I assumed it was not optimized that way. :)

result.push(color);
};

Expand Down