Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on implementing the core functionality! The application works as expected, and the component structure is solid. However, I'm requesting a few changes to align the code with the project's quality standards.
There are a couple of violations of the coding checklist that need to be addressed:
- One-letter variable names: In
src/App.tsx, variables likee,t, anduare used. Please rename them to be more descriptive, such asevent,todo, anduser. This improves code readability and maintainability. [CHECKLIST ITEM #1] - Code Duplication: The logic to find a user by their ID is repeated in
src/App.tsx. Please extract this logic into a single, reusable helper function to adhere to the DRY (Don't Repeat Yourself) principle. [CHECKLIST ITEM #2]
Once these issues are resolved, your solution will be ready for approval. Keep up the great work!
✨ 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
| @@ -1 +1,12 @@ | |||
| export const TodoList = () => {}; | |||
| import { TodoInfo } from '../TodoInfo'; | |||
| import { Props } from '../../types/types'; | |||
There was a problem hiding this comment.
It's a good practice to define component-specific props within the component's file. The Props type defined in types/types.ts is only used by TodoList. It would be better to move this type definition into this file to make the component more self-contained. The types.ts file should be reserved for types that are shared across multiple components, such as Todo and User.
src/App.tsx
Outdated
| // import usersFromServer from './api/users'; | ||
| // import todosFromServer from './api/todos'; | ||
| const getTodoWithUser = (todo: Omit<Todo, 'user'>): Todo => { | ||
| const user = usersFromServer.find((u: User) => u.id === todo.userId) || null; |
There was a problem hiding this comment.
This violates checklist item #1: 'Don't use one-letter variable naming'. Please use a more descriptive name for the callback parameter, for example, user instead of u.
src/App.tsx
Outdated
| const [titleError, setTitleError] = useState(false); | ||
| const [userError, setUserError] = useState(false); | ||
|
|
||
| const handleSubmit = (e: React.FormEvent) => { |
There was a problem hiding this comment.
This violates checklist item #1: 'Don't use one-letter variable naming'. Please use a more descriptive name for the event object, for example, event instead of e.
src/App.tsx
Outdated
| } | ||
|
|
||
| const newTodo: Todo = { | ||
| id: Math.max(...todos.map(t => t.id)) + 1, |
There was a problem hiding this comment.
This violates checklist item #1: 'Don't use one-letter variable naming'. Please use a more descriptive name for the callback parameter, for example, todo instead of t.
src/App.tsx
Outdated
| title, | ||
| userId, | ||
| completed: false, | ||
| user: usersFromServer.find(u => u.id === userId) as User, |
There was a problem hiding this comment.
This line has a couple of issues:
- It violates checklist item Solution #1: 'Don't use one-letter variable naming'. The parameter
ushould have a more descriptive name likeuser. - It violates checklist item 1 #2: 'Don't repeat yourself'. The logic to find a user by their ID is also present in the
getTodoWithUserfunction. It's better to extract this logic into a reusable helper function.
src/App.tsx
Outdated
| <select | ||
| data-cy="userSelect" | ||
| value={userId} | ||
| onChange={e => { |
There was a problem hiding this comment.
This violates checklist item #1: 'Don't use one-letter variable naming'. Please use a more descriptive name for the event object, for example, event instead of e.
| export type Props = { | ||
| todos: Todo[]; | ||
| }; |
There was a problem hiding this comment.
This Props type is only used by the TodoList component. It's a best practice to define props specific to a single component within that component's file. This improves component encapsulation and makes the codebase easier to navigate and maintain. Please move this type definition to src/components/TodoList/TodoList.tsx.
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 task! You've successfully addressed all the previous feedback, and the code is clean and well-structured. I am requesting one small change to make the application more robust before approval.
The current method for generating a new todo ID in src/App.tsx does not handle the case where the todos list is empty.
- Issue:
Math.max(...todos.map(t => t.id))returns-Infinityfor an empty array, resulting in an invalid ID for the first todo item. - Suggestion: Please add logic to handle this edge case. A simple approach is to default the ID to
1if thetodosarray is empty.
Once you've made this fix, the solution will be perfect. Keep up the great work!
✨ 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/App.tsx
Outdated
| } | ||
|
|
||
| const newTodo: Todo = { | ||
| id: Math.max(...todos.map(todo => todo.id)) + 1, |
There was a problem hiding this comment.
This method of generating an ID works correctly as long as the todos array is not empty. However, if todos were empty, Math.max() called without arguments would return -Infinity, causing issues with the new id.
A more robust approach would be to handle this edge case, for example, by providing a default starting value if the array is empty.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work! I am approving your solution. You've successfully resolved the issue from the previous review, and the logic for generating a new todo ID now correctly handles all edge cases. The overall solution is clean, well-structured, and demonstrates a solid understanding of React state management and form handling. All components are implemented correctly, and the application meets all functional requirements. Great job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
DEMO LINK