London | 26-SDC-March | Zobeir Rigi | Sprint 2 | Chat app#101
London | 26-SDC-March | Zobeir Rigi | Sprint 2 | Chat app#101Zobeir-Rigi wants to merge 30 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
Zobeir-Rigi
added
📅 Sprint 2
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
removed
the
Needs Review
Zobeir-Rigi
changed the title
Zobeir-Rigi
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
The README has this requirement:
It must also support at least one additional feature.
In the PR description, can you list the extra features you implemented?
| const allMessages = [] | ||
|
|
||
| const fetchAllMessages = () => { | ||
| return allMessages | ||
| } | ||
|
|
||
| app.get("/messages", (req, res) => { | ||
| const messages = fetchAllMessages() | ||
| res.json(messages) | ||
| }) | ||
|
|
||
| const clients = []; |
There was a problem hiding this comment.
Could consider moving these declaration before line 5 to keep all declarations before executable statements.
cjyuan
added
Reviewed
Yes, I also added validation and a timestamp. |
…so for null values
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Changes look good.
I could not find these features listed in the PR description though.
| } | ||
| } | ||
|
|
||
| fetchMessages() |
There was a problem hiding this comment.
I think it is a better practice to place all the code to be executed when this script is loaded in one place. For example, in a function called init() or at the end of the file (after all declarations and function definitions). It would make locating them easier.
There was a problem hiding this comment.
Good point, I added an init() function to group what runs when the app loads, and I put fetching messages there as part of the startup.
| const data = await res.json(); | ||
|
|
||
| if (!res.ok) { | ||
| showError(data.error); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Could the server return a response not generated by your service API, and as such return a response that is not a JSON encoded message? The error may not necessary be a "Network error".
It could be worth researching how to properly handle a response error in such a scenario.
No change needed.
There was a problem hiding this comment.
Thanks, I really appreciated the guidance – it was clear and helped me improve the code in a clean way.
cjyuan
added
Complete
|
Closing PR because the SDC run has finished. Feel free to re-open if you're still working on it. |
3 participants
Self checklist
Built a full-stack chat application using Express and WebSockets, supporting GET and POST APIs for messages, real-time updates, and input validation on both frontend and backend.
I added validation and timestamps as extra features. This is an MVP that meets the requirements for this PR, and I’ll improve it further when I get some time.
server
client