Feedback on initial implementation #1
Conversation
| const bodyBytes = []; | ||
|
|
||
| // It's quite hard to read what this function is doing, because it does lots of things at different levels of abstraction. | ||
| // Can you think how to make this function only act at one level of abstraction (e.g. by using a middleware for some of the handling)? |
There was a problem hiding this comment.
yes, I can use express.json() to do exactly the same job
| } | ||
| const newMessage = { | ||
| messageText: body.messageText, | ||
| // Can you think of any problems with using a user-supplied timestamp here, vs calculating the timestamp on the server? |
There was a problem hiding this comment.
The user can modify the timestamp on the client side, so we shouldn't trust any user-provided input for security reasons.
| console.log(new Date() + " Connection accepted."); | ||
| activeWsConnections.push(connection); | ||
| connection.on("close", function (reasonCode, description) { | ||
| // This looks like a TODO you meant to do? |
There was a problem hiding this comment.
done, now I use splice() to remove connection from array of connections
| ); | ||
|
|
||
| // It looks like you're using websockets just to notify "there's something for you to fetch", which feels slower than including the messages in the websocket itself. | ||
| // What do you think? |
There was a problem hiding this comment.
wow, I didn’t know that a message can be sent to the connection right after it’s established. Done
| const messageText = inputMessage.value.trim(); | ||
|
|
||
| if (!messageText) { | ||
| // This feels like it would be handy to extract a named function for "show an error message and then remove it". |
|
|
||
| websocket.addEventListener("message", (mesEvent) => { | ||
| const response = JSON.parse(mesEvent.data); | ||
| // How unique do you expect timestamps to be? Can you see any risks with this approach to identifying messages? |
There was a problem hiding this comment.
yes, it's possible for two users to send message at exactly the same millisecond. I updated the message in state check to compare the entire message instead, but it would probably be better to assign a unique ID to each message.
2 participants
One other user-facing thing - when I pressed the react buttons on https://droid-an-chat-application-frontend.hosting.codeyourfuture.io/ I found the initial click wasn't always reflected in the UI. If I click the thumbs down button 6 times, I only see 3 updates - when it updates, it shows the correct number, but half of the clicks don't instantly show the new number?