Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I’m requesting changes because there is a critical validation bug that can block correct behavior for specified inputs. In src/checkRequest.js, lines 28-29 perform an early return after adding the toCase-missing error; this prevents collecting multiple errors when both text and toCase are missing, which violates the requirement to return all validation failures. Additionally, the if (!toCase) check (around lines 25-29) conflates missing and invalid values and can interfere with proper validation flow; the function should distinguish between a truly missing toCase and an invalid toCase value so the invalidToCase check can run when appropriate. To fix this, remove the early return so both errors can be accumulated, and adjust the condition to check specifically for toCase === undefined (or similar) before proceeding to the invalid-case check. The rest of the server logic (parsing, invoking convertToCase, response shapes, headers, and status codes) looks solid and aligns with the requirements. Minor note: using the conventional status text 'Bad Request' (capital R) is stylistic and not a blocker. Keep up the good work—once the validation logic is corrected, this should be good to go.
✨ 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
| }, | ||
| noToCase: { | ||
| message: |
There was a problem hiding this comment.
Checklist validation: The code correctly exports createServer (checklist items #1, #14) and returns an http server created via http.createServer (items #2, #15). It parses the request using parseRequest (line 10), validates using checkRequest (line 11), and calls convertToCase (line 19) when validation passes (items #7, #26, #27). The response objects and headers match the required JSON structure and Content-Type header (lines 28-29 and 14-16), and status codes/statusText are used as required (lines 14 and 28).
| const checkRequest = (text, toCase) => { | ||
| const errors = []; | ||
|
|
||
| if (!text) { | ||
| errors.push(errorCases.noText); | ||
| } | ||
|
|
||
| if (!toCase) { | ||
| errors.push(errorCases.noToCase); | ||
|
|
||
| return errors; |
There was a problem hiding this comment.
Validation logic is implemented in src/checkRequest.js and returns an array of error objects matching the required messages (lines 3, 4-17, 22-36). It correctly accumulates both missing text and missing toCase when appropriate and returns immediately after adding the toCase missing error (line 29) which matches intended behavior since further validation of toCase value isn't meaningful when it's missing (checklist items #19-#25).
| '"toCase" query param is required. Correct request is: "/<TEXT_TO_CONVERT>?toCase=<CASE_NAME>".', | ||
| }, | ||
| invalidToCase: { | ||
| message: |
There was a problem hiding this comment.
Minor suggestion: In createServer.js you return the response with writeHead including status text strings 'Bad request' and 'OK' which are acceptable, but note Node's conventional status message for 400 is 'Bad Request' (capital R). Tests likely check numeric status and body, so this is not a functional failure—just a stylistic note.
|
I don't think the suggested change is necessary here. The early return does not prevent collecting both errors when errors.push(errorCases.noText);
errors.push(errorCases.noToCase);
return errors;As a result, both required errors are returned. The early return is intentional: once Also, For these reasons, I believe the current validation flow satisfies the requirements. |
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because the validation in src/checkRequest.js conflates a missing toCase with an invalid value and returns early, which violates explicit requirements to distinguish these cases and to accumulate multiple errors. Specifically, the check uses a falsy condition (if (!toCase)) and returns immediately, preventing the invalidToCase check from running when toCase is present but invalid and risking incorrect handling of empty strings or null from URLSearchParams.get() [CHECKLIST ITEMS #18, #19, #22, #29, #30]. To fix this:
- Detect missing with
toCase == null(coversnullandundefined) and pushnoToCasewithout returning. - Only if
toCase != null, run!supportedCases.includes(toCase)and pushinvalidToCase. - Remove the early return so both
noTextandnoToCasecan be collected when both are absent.
The rest ofcreateServerandmain.jslooks solid and aligned with the requirements. Once the missing-vs-invalid logic is corrected, the implementation should meet the task standards.
✨ 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
| if (!toCase) { | ||
| errors.push(errorCases.noToCase); | ||
|
|
||
| return errors; |
There was a problem hiding this comment.
checkRequest currently returns early when toCase is falsy. This conflates "missing" and "invalid" states and prevents the invalid-case check from running when appropriate. According to the requirements you must: (a) report noToCase when toCase is truly missing, and (b) report invalidToCase when toCase is present but not one of the supported cases. Also avoid the early return so multiple errors can be collected when applicable. Consider specifically testing for toCase == null (which covers both null and undefined) to detect a missing param, and only run the supportedCases.includes check when toCase is not null. This aligns with checklist items #18, #19, #22, #29 and #30.
| errors.push(errorCases.noText); | ||
| } | ||
|
|
||
| if (!toCase) { |
There was a problem hiding this comment.
Because URLSearchParams.get() returns null when a parameter is missing, use a missing check like toCase == null rather than !toCase. That prevents treating empty-string or other falsy-but-present values the same as missing. Then, only if toCase != null run supportedCases.includes(toCase) to determine invalid-toCase.
| convertedText: result.convertedText, | ||
| }; | ||
|
|
||
| res.writeHead(200, 'OK', { 'Content-Type': 'application/json' }); | ||
| res.end(JSON.stringify(response)); |
There was a problem hiding this comment.
The check here uses a falsy check and returns immediately after pushing the noToCase error. This conflates missing and invalid cases. According to the task you must distinguish a truly missing toCase from an invalid value and allow collecting multiple errors (e.g. when both text and toCase are missing). Change this check to explicitly detect a missing param (for example toCase == null which covers null and undefined), remove the early return so multiple errors can be accumulated, and only run the invalidToCase check when toCase is present.
| return server; | ||
| } |
There was a problem hiding this comment.
The supported-cases membership check is fine, but it should only execute when toCase is present. After adjusting the missing check as suggested, this includes check will behave correctly.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Here is my approval with an educational override to help you progress while learning. You’ve done a great job overall: the server is correctly created and exported, parses requests, invokes the business logic, and responds with proper JSON and headers. The main issue to address is in src/checkRequest.js: at lines where you validate parameters, using if (!toCase) conflates missing vs. invalid values and can prevent accumulating multiple errors. Per requirements, separate the conditions as: if toCase == null then add the missing error; else if !supportedCases.includes(toCase) then add the invalid-value error. Similarly, use text == null instead of !text to detect a truly missing text value, and avoid early returns so both errors can be reported together [CHECKLIST ITEM #19, #20, #23]. Minor: keep error ordering consistent (e.g., text first, then toCase-related) for predictable outputs. Overall, strong work—these small validation tweaks will make your solution fully compliant and more robust.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| errors.push(errorCases.noText); | ||
| } | ||
|
|
||
| if (!toCase) { |
There was a problem hiding this comment.
The current check uses a falsy check if (!toCase) which conflates missing toCase (null) and other falsy values. Per requirements you must detect a missing parameter with toCase == null and only when toCase != null check if it's supported. Also avoid early returns so multiple errors (e.g., missing text and missing toCase) can be returned together.
|
|
||
| if (!toCase) { | ||
| errors.push(errorCases.noToCase); | ||
| } else if (!supportedCases.includes(toCase)) { |
There was a problem hiding this comment.
Because of the falsy check above, an invalid value for toCase (e.g. toCase=UNKNOWN) will be correctly detected only when toCase is present; however the logic should explicitly separate the missing-vs-invalid cases as described in the task and previous review guidance. Change the condition to: if (toCase == null) { errors.push(...) } else if (!supportedCases.includes(toCase)) { errors.push(...) }.
| targetCase: toCase, | ||
| originalText: text, | ||
| convertedText: result.convertedText, | ||
| }; |
There was a problem hiding this comment.
This check treats any falsy toCase (including empty string) as missing and immediately skips the invalid-value check. Per requirements you must detect missing toCase using toCase == null (so only null/undefined are considered missing) and, when toCase is present but not supported, add the invalidToCase error. Also ensure you do not return early so errors for both missing text and missing/invalid toCase can be accumulated. Update the conditions accordingly (do not use if (!toCase)).
| convertedText: result.convertedText, | ||
| }; | ||
|
|
||
| res.writeHead(200, 'OK', { 'Content-Type': 'application/json' }); |
There was a problem hiding this comment.
Because the if (!toCase) branch prevents executing the invalidToCase check when toCase is present but falsy, change the logic to: if (toCase == null) { push noToCase } else if (!supportedCases.includes(toCase)) { push invalidToCase }. This will satisfy checklist items #19, #20 and #23 (accumulating errors).
No description provided.