london-10-shadi-fakhri-full-stack-project

[Shadi38]

No description provided.

[illicitonion]

This is looking really good, congrats!

[illicitonion]

Is there a more clear name than loadData you can think of for this state that describes what data is stored in this array?

Generally all apps deal with "data", so being specific can make things a lot more clear :)

[Shadi38]

Hi Daniel
thanks for your pieces of advice. I will try to do all of them.
here i changed loadData to loadVideo.

[illicitonion]

This code looks a lot like the code in aseClickHandler - can you work out how to share code between them, rather than copying it?

[Shadi38]

you are right I fixed it.

[illicitonion]

Once you've loaded the data once, you probably don't actually need to load it again just to change the sort order - can you make this re-sort without fetching again?

[Shadi38]

I deleted fetch from the function.

[illicitonion]

loadData here sounds like it's probably a function you can call to load data, because its name starts with a verb. Can you think of a more clear name for this prop?

[Shadi38]

I changed it.

[illicitonion]

I'm not sure the Data suffixes on these state names are adding much - maybe remove them?

[Shadi38]

I changed them to title and url

[illicitonion]

It's pretty common to only 404 if we're looking for one specific thing that couldn't be found, and if you're asking for "all the videos", to just return the empty array and let the client handle that. What you've done here isn't wrong, it's totally valid, just raising that there are alternatives too :) Can you think of some advantages/disadvantages of both approaches?

[Shadi38]

thanks for the good advice. I changed that.

[illicitonion]

Why do you have a second handler function for the same method on the same path? Which one will end up getting used?

[illicitonion]

Can you do the sorting in SQL rather than in javascript? (Both work, but doing it in SQL will probably be a bit faster :))

[Shadi38]

I did it.

[illicitonion]

This doesn't use your database - can you rewrite this to do a SQL query (and like with the sorting, to do the filtering in SQL not JS? Again - the JS still works, but doing it in SQL will probably be faster).

[illicitonion]

Can you do this in SQL? :)

[Shadi38]

I did it in Sql

[Shadi38]

thanks, Daniel for reviewing my project. your advice was really good and helped my project to get better. I tried to do all of them. I appreciate it if you could check my commits and let me know your opinion of them.

…

On 19 Sept 2023, at 00:05, Daniel Wagner-Hall ***@***.***> wrote: @illicitonion commented on this pull request. This is looking really good, congrats! In client/src/App.js <#375 (comment)>: > function App() { + + const [showVideos, setShowVideos] = useState(false); + const [loadData, setLoadData] = useState([]); Is there a more clear name than loadData you can think of for this state that describes what data is stored in this array? Generally all apps deal with "data", so being specific can make things a lot more clear :) In client/src/App.js <#375 (comment)>: > + const response = await fetch( + "https://web-server-5nme.onrender.com/videos" + ); + if (!response.ok) { + throw new Error("something went wrong"); + } + const data = await response.json(); + //asending acording to the rating + data.sort((a, b) => a.rating - b.rating); + return setLoadData(data); + } catch (error) { + console.error("Error fetching data:", error); + } + } + +async function descClickHandler(e) { This code looks a lot like the code in aseClickHandler - can you work out how to share code between them, rather than copying it? In client/src/App.js <#375 (comment)>: > + if (!response.ok) { + throw new Error("something went wrong"); + } + const data = await response.json(); + //asending acording to the rating + data.sort((a, b) => a.rating - b.rating); + return setLoadData(data); + } catch (error) { + console.error("Error fetching data:", error); + } + } + +async function descClickHandler(e) { + e.preventDefault(); + try { + const response = await fetch("https://web-server-5nme.onrender.com/videos"); Once you've loaded the data once, you probably don't actually need to load it again just to change the sort order - can you make this re-sort without fetching again? In client/src/Videos.js <#375 (comment)>: > @@ -0,0 +1,277 @@ +import React, { useEffect, useState } from "react"; +import ThumbUpIcon from ***@***.***/icons-material/ThumbUp"; +import ThumbDownIcon from ***@***.***/icons-material/ThumbDown"; +function Videos(props) { + const {loadData,setLoadData}=props; loadData here sounds like it's probably a function you can call to load data, because its name starts with a verb. Can you think of a more clear name for this prop? In client/src/Videos.js <#375 (comment)>: > + const [titleData, setTitleData] = useState(""); + const [urlData, setUrlData] = useState(""); I'm not sure the Data suffixes on these state names are adding much - maybe remove them? In client/src/Videos.js <#375 (comment)>: > + .then((data) => { + const updatedData = loadData.map((video) => { + if (video.id === item.id) { + return { ...video, rating: newRating }; + } + return video; + }); + console.log(updatedData); + setLoadData(updatedData); + }) + .catch((error) => { + console.error("Error updating video:", error); + }); + } + + function thumbDownHandeler(item) { This function looks a lot like thumbUpHandeler - can you make them be one function? In server/server.js <#375 (comment)>: > +const { body, validationResult } = require("express-validator"); +const port = process.env.PORT || 3000; +const { Pool } = require("pg"); + +// const db = new Pool({ +// user: "shadifakhri", +// host: "localhost", +// database: "database", +// password: "", +// port: 5432, +// }); + +const db = new Pool({ + user: "shadi_user", + host: "dpg-cjs7s5dv2qks738v6ltg-a.oregon-postgres.render.com", + database: "shadi", You shouldn't check your password in here - you probably want to remove it, change the password, and use a .env file which you .gitignore for the password. In server/server.js <#375 (comment)>: > -// GET "/" -app.get("/", (req, res) => { - // Delete this line after you've confirmed your server is running - res.send({ express: "Your Backend Service is Running" }); +// GET "/videos" +app.get("/videos", async function (req, res) { + const result = await db.query("SELECT * FROM videos"); + if (result.rows.length === 0) { + return res.status(404).json({ error: "no videos found" }); It's pretty common to only 404 if we're looking for one specific thing that couldn't be found, and if you're asking for "all the videos", to just return the empty array and let the client handle that. What you've done here isn't wrong, it's totally valid, just raising that there are alternatives too :) Can you think of some advantages/disadvantages of both approaches? In server/server.js <#375 (comment)>: > +// return res.status(404).json({ error: "no videos found" }); +// } +// const order = req.query.order; + +// let orderVideos; +// if (order === "asc") { +// orderVideos = videos.sort((a, b) => a.rating - b.rating); +// } else { +// // Default to descending order if order is not specified or is invalid +// orderVideos = videos.sort((a, b) => b.rating - a.rating); +// } + +// return res.json(orderVideos); +// }); +//ordering by assending and dessending for example /videos?order=asc +app.get("/videos", async function (req, res) { Why do you have a second handler function for the same method on the same path? Which one will end up getting used? In server/server.js <#375 (comment)>: > +// } +// const order = req.query.order; + +// let orderVideos; +// if (order === "asc") { +// orderVideos = videos.sort((a, b) => a.rating - b.rating); +// } else { +// // Default to descending order if order is not specified or is invalid +// orderVideos = videos.sort((a, b) => b.rating - a.rating); +// } + +// return res.json(orderVideos); +// }); +//ordering by assending and dessending for example /videos?order=asc +app.get("/videos", async function (req, res) { + const result = await db.query("SELECT * FROM videos"); Can you do the sorting in SQL rather than in javascript? (Both work, but doing it in SQL will probably be a bit faster :)) In server/server.js <#375 (comment)>: > + "INSERT INTO videos (title, url, rating) VALUES($1, $2, $3) RETURNING *"; + db.query(query, [newTitle, newUrl, newRating], (err,result) => { + if (err) { + res.status(500).send("Internal Server Error"); + } else { + const createdVideo = result.rows[0]; + res.status(201).json(createdVideo); + } + }); + } +); + +//search video by query for example /videos/search?title=halleluja +app.get("/videos/search", function (req, res) { + const searchVideo = req.query.title; + const filteredVideo = videos.filter((video) => This doesn't use your database - can you rewrite this to do a SQL query (and like with the sorting, to do the filtering in SQL not JS? Again - the JS still works, but doing it in SQL will probably be faster). In server/server.js <#375 (comment)>: > +// res.status(200).json( video ); +// }); + +//search video by id for example/videos/:id +app.get("/videos/:id", async function (req, res) { + const videoId=req.params.id; + const result = await db.query('SELECT * FROM videos where id=$1',[videoId]); + if (result.rows.length === 0) { + return res.status(404).json({ error: "no videos found" }); + } + res.json(result.rows); +}); + +//updating video with id +app.put("/videos/:id", function (req, res) { + const video = videos.find((m) => m.id == req.params.id); Can you do this in SQL? :) — Reply to this email directly, view it on GitHub <#375 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A276C22CMQJCLNSF26OXRULX3DHT7ANCNFSM6AAAAAA37UEQME>. You are receiving this because you authored the thread.