add task solution #5219
add task solution #5219
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job implementing the core logic correctly and avoiding mutations! However, a few changes are needed to meet the specific structural requirements of the task. The main issue is the use of if statements instead of a switch statement to handle the different action types [CHECKLIST ITEM #2]. When you refactor this, please also include a default case for handling unknown actions [CHECKLIST ITEM #3]. Finally, the logic for pushing to history and updating the state is repeated in each block; this should be moved outside to run once after the conditional logic [CHECKLIST ITEM #4]. Addressing these points will get your solution ready for approval!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
src/transformStateWithClones.js
Outdated
| if (actions[i].type === 'addProperties') { | ||
| const action = actions[i]; | ||
|
|
||
| const nextState = Object.assign({}, prevState, action.extraData); | ||
|
|
||
| history.push(nextState); | ||
|
|
||
| prevState = nextState; | ||
| } | ||
|
|
||
| if (actions[i].type === 'removeProperties') { | ||
| const nextState = Object.assign({}, prevState); | ||
| const action = actions[i]; | ||
|
|
||
| for (const k of action.keysToRemove) { | ||
| delete nextState[k]; | ||
| } | ||
|
|
||
| history.push(nextState); | ||
|
|
||
| prevState = nextState; | ||
| } | ||
|
|
||
| if (actions[i].type === 'clear') { | ||
| const nextState = {}; | ||
|
|
||
| history.push(nextState); | ||
|
|
||
| prevState = nextState; | ||
| } |
There was a problem hiding this comment.
This block of if statements should be refactored into a switch statement to handle the different action types. This violates a few checklist items:
- Checklist item sol #2: 'Use switch statement if you have limited amount of conditions.'
- Checklist item Kisilov_Vadim #3: 'switch/case should always have default case for error handling.' Your implementation is missing handling for unknown action types.
- Checklist item Solution #4: 'If you perform same action in all
switchcases - do it just once afterwards.' The code for pushing tohistoryand updatingprevStateis repeated in each block. This logic should be extracted and run once after theswitchstatement within the loop.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this iteration! You've successfully addressed all the feedback from the previous review, and the code is now clean, well-structured, and fully compliant with the requirements. Using a switch statement and moving the common logic outside of it was a great improvement. Your solution correctly handles all state transformations without mutating the original data. I am happy to approve this submission.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.