Skip to content

Conversation

@areebsattar
Copy link

@areebsattar areebsattar commented Apr 27, 2024

Learners, PR Template

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with COHORT_NAME | FIRST_NAME LAST_NAME | REPO_NAME | WEEK
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

Briefly explain your PR.

Level 1 completed for chat server

Questions

Ask any questions you have for your reviewer.

@sonarqubecloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

@garydev10 garydev10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The coding is clear and readable. Good job! It can achieve Level 1 after some change.


app.post("/messages", (req, res) => {
const newMessage = req.body;
messages.push(newMessage);
Copy link

@garydev10 garydev10 Apr 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good grasp of Create (POST) function!
However, does reg.body contain all properties required in the message? (See Data Model)

app.get("/messages/:id", (req, res) => {
const messageId = parseInt(req.params.id);
const message = messages.find((obj) => obj.id === messageId );
if (!message) return res.status(404).json({ message: "Message does not exist" });

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though your code here is correct and works, a lot of programmers will always add {}s around if bodies, even if they're just one statement - have a read of https://www.synopsys.com/blogs/software-security/understanding-apple-goto-fail-vulnerability-2.html for some explanation as to why :)


app.delete("/messages/:id", (req, res) => {
const index = messages.findIndex((obj) => obj.id === parseInt(req.params.id));
if (index === -1) return res.status(404).json({ message: "No message to delete" });

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though your code here is correct and works, a lot of programmers will always add {}s around if bodies, even if they're just one statement - have a read of https://www.synopsys.com/blogs/software-security/understanding-apple-goto-fail-vulnerability-2.html for some explanation as to why :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants