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 server and handling the core logic! I am requesting a couple of changes to make your solution more robust and fully compliant with the requirements.
First, there's a critical issue that can crash the server. If a request is made without a query string (e.g., /some-text), your URL parsing logic at src/createServer.js will cause a TypeError because urlParams[1] will be undefined. Please add a check or a default value to handle this case gracefully.
Additionally, the task requires the success status message to be OK, but your code currently sends ok. Please update this to match the specification.
Once you've fixed these points, your server will be solid!
✨ 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/createServer.js
Outdated
| return http.createServer((req, res) => { | ||
| const urlParams = req.url.split('?'); | ||
| const text = urlParams[0].slice(1); | ||
| const queryString = urlParams[1]; |
There was a problem hiding this comment.
This line might lead to a server crash. If the request URL doesn't have a query string (no ?), urlParams[1] will be undefined. Passing this to URLSearchParams on line 10 will throw a TypeError. How could you provide a default value for queryString to prevent this?
src/createServer.js
Outdated
| if (errors.length > 0) { | ||
| res.statusCode = 400; | ||
| res.statusMessage = 'Bad request'; | ||
| res.setHeader('Content-type', 'application/json'); |
There was a problem hiding this comment.
While HTTP headers are case-insensitive, it's a good practice to be consistent with their casing. Here you've used Content-type, but on line 54 you used Content-Type. Sticking to the standard Content-Type is recommended.
src/createServer.js
Outdated
| const result = convertToCase(text, toCase); | ||
|
|
||
| res.statusCode = 200; | ||
| res.statusMessage = 'ok'; |
There was a problem hiding this comment.
The task requirements specify that the success status text should be OK, not ok.
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! I am approving your solution as you've successfully addressed all the feedback from the previous review. The server now correctly handles requests without query strings, and the status messages are fully compliant with the requirements. Your code is clean, robust, and meets all the task specifications. Great job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.