-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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.
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
solution #2682
base: master
Are you sure you want to change the base?
solution #2682
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! There are things that should be fixed:
src/transformState.js
Outdated
break; | ||
|
||
case 'removeProperties': | ||
if (actions[i].keysToRemove !== undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need for checking if 'keysToRemove' is undefined. If it's undefined, the later for loop just won't execute.
src/transformState.js
Outdated
@@ -5,7 +5,34 @@ | |||
* @param {Object[]} actions | |||
*/ | |||
function transformState(state, actions) { | |||
// write code here | |||
for (let i = 0; i < actions.length; i++) { | |||
const mainAction = actions[i].type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of traditional for loop, use of 'for...of' loop is recommended for better readability when iterating over an array.
src/transformState.js
Outdated
@@ -5,7 +5,34 @@ | |||
* @param {Object[]} actions | |||
*/ | |||
function transformState(state, actions) { | |||
// write code here | |||
for (let i = 0; i < actions.length; i++) { | |||
const mainAction = actions[i].type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Destructure action
const { type, extraData, keysToRemove } = actions[i]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost done!
src/transformState.js
Outdated
break; | ||
|
||
case 'removeProperties': | ||
const toRemove = keysToRemove; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this variable is reduntant here
src/transformState.js
Outdated
for (let j = 0; j < toRemove.length; j++) { | ||
delete state[toRemove[j]]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't see any sense in using for loop, better to use for of
or forEach
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small but important fix left 🙏
src/transformState.js
Outdated
const { type, extraData, keysToRemove } = action; | ||
|
||
switch (type) { | ||
case 'clear': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move all cases to the object.
Example of usage:
case ActionType.Clear:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job!
No description provided.