Skip to content

NW5-Manchester-Khadar-Prep-work-Node-W3#17

Open
khmdagal wants to merge 3 commits intoCodeYourFuture:masterfrom
khmdagal:master
Open

NW5-Manchester-Khadar-Prep-work-Node-W3#17
khmdagal wants to merge 3 commits intoCodeYourFuture:masterfrom
khmdagal:master

Conversation

@khmdagal
Copy link

master

Copy link

@Ara225 Ara225 left a comment

Choose a reason for hiding this comment

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

Very nice :)


// reading All albums
app.get("/albums", (req, res) => {
fs.readFile("./albums.json", "utf-8").then((movie) => {
Copy link

Choose a reason for hiding this comment

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

You don't write to this file, so it doesn't neccessarily make sense to read it everytime you make a request. require() will do the job

const id = +req.params.id;
const index = fs.readFile("./albums.json", "utf-8").then((movie) => {
const searchedAlbum = JSON.parse(movie).findIndex(
(album) => +album.id === +req.params.id
Copy link

Choose a reason for hiding this comment

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

I prefer to be a little more explict with my number conversions, this syntax is a little hard to read quickly, but this is a matter of personal preference

const newAlbum = {
album: requestBody.album,
artist: requestBody.artist,
id: theOldAlbum.id, // here I want don't want to change the old id
Copy link

Choose a reason for hiding this comment

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

Good :)

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