NW6 | Fikret Ellek| Module Servers | Chat server API project | Week 2#180
NW6 | Fikret Ellek| Module Servers | Chat server API project | Week 2#180fikretellek wants to merge 22 commits intoCodeYourFuture:mainfrom
Conversation
|
server is live on ---> https://module-servers.onrender.com/ |
| "main": "server.js", | ||
| "scripts": { | ||
| "start": "node server.js" | ||
| "start": "nodemon server.js" |
There was a problem hiding this comment.
Nice touch using nodemon! This can save you a lot of time and effort during development. Well done!
chat-server/server.js
Outdated
| typeof newMessage.text != "string" || | ||
| typeof newMessage.from != "string" | ||
| ) { | ||
| res.sendStatus(400).send("wrong input"); |
There was a problem hiding this comment.
One suggestion for improvement is that instead of using res.sendStatus(400).send("wrong input"), you can use res.status(400).send("wrong input"). res.status(400) set the status code and then send the error message using send.
There was a problem hiding this comment.
Ahh I see, that makes more sense. Thank you.
chat-server/server.js
Outdated
| newMessage.text === "" || | ||
| typeof newMessage.text != "string" || | ||
| typeof newMessage.from != "string" | ||
| ) { |
There was a problem hiding this comment.
If you want to go one further step, you can create your validation fun and give single responsibility to it. It looks like this:
function validateMessage(newMessage) {
//your validation returns boolean
}
app.post("/messages", (req, res) => {
const newMessage = req.body;
const isMessageValid = validateMessage(newMessage
if (!isMessageValid)) {
res.status(400).send("wrong input");
return;
}
// Rest of your code...
There was a problem hiding this comment.
I have modified the route function. Could you have a look please?
chat-server/server.js
Outdated
| res.json(messages); | ||
| }); | ||
|
|
||
| app.get("/messages/id/:id", (req, res) => { |
There was a problem hiding this comment.
There is no right and wrong, but you generally don't need to include the word 'id' in the route path. you can simply use /messages/:id. It makes the endpoint more concise and follows a common convention in RESTful APIs.
There was a problem hiding this comment.
I really had a bad time fixing this 😆 when I tried to create another route (ex: /message/search) it was getting recognised as :id parameter. I couldn't solve the problem and found this solution. After seeing this message spent an hour but still couldn't solve 😆
chat-server/server.js
Outdated
| res.json(searchedMessage); | ||
| }); | ||
|
|
||
| app.delete("/messages/id/:id", (req, res) => { |
There was a problem hiding this comment.
Can we set up a meeting for this issue? I couldn't find how to resolve dynamic and static path blocking
chat-server/server.js
Outdated
| }); | ||
|
|
||
| app.get("/messages/latest", (req, res) => { | ||
| const latestMessages = messages.slice(-10); |
There was a problem hiding this comment.
I'm sure you can to this more dynamic instead of using hardcoded length 💯
There was a problem hiding this comment.
yes it makes more sense. I have updated the route with /messages/latest/:length route
…on fails (other wise failing message was getting added too)
…hs are blocking static ones)
musayuksel
left a comment
musayuksel
left a comment
There was a problem hiding this comment.
Hey @fikretellek ,
This is fantastic! Moving the validation logic to a separate function has already made the code significantly more readable and easier to test.
Here's an additional step you can explore to further improve the structure and maintainability of the code: it's called middleware. Don't worry, it's not your responsibility at this point, but I can see your potential and I'd love to introduce you to it for when you have some extra time to learn.
Middleware in Express allows you to create reusable functions that intercept incoming requests and perform actions before they reach the route handler. This is a great way to for common tasks like validation, authentication.
Here's an example of how you could implement message validation as middleware:
const messageValidationMiddleware = (req, res, next) => {
const message = req.body;
const isMessageValid = validateMessage(message);
if (isMessageValid) {
next(); // Pass it to the next middleware or route handler in your scenario
} else {
const errorMessage = new Error('Your message input is invalid :( ');
next(errorMessage); // Pass the error to the global error handler
// IT WILL SKIP ALL OTHER MIDDLEWARES AND ROUTES
}
};
// Now, in your route handler, you can remove the validation logic:
app.post('/messages', messageValidationMiddleware, (req, res) => {
const newMessage = req.body;
// Since the message has already been validated, we can safely assign an ID
newMessage['id'] = messages[messages.length - 1].id + 1;
// ... rest of your code
});
// ... OTHER ROUTES
const globalErrHandler = (err, req, res, next) => {
res.status(400).send(err.message); // comes from any middleware,
// in this code it is `Your message input is invalid :(`
// The status code can be dynamic in real applications, but that is beyond the scope of this explanation
};
app.use(globalErrHandler);
app.listen(process.env.PORT, () => {
console.log(`listening on PORT ${process.env.PORT}...`);
});
- The globalErrHandler function takes four arguments: err, req, res, and next.
- When we can call
next(err)in one of our middleware function, it will skip any remaining middleware and route handlers and jump straight to theglobalErrHandlermiddleware. - The globalErrHandler receives the error object as the first argument (
Error). - By utilising the
globalErrHandler,we ensure std error handling throughout our app.
Remember, this is just an optional step you can explore for further learning. The current implementation is already great! Keep up the excellent work, @fikretellek .
Co-authored-by: Jay Mayer <JayMayer@users.noreply.github.com>
|
2 participants
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.