London_10 | Mehmet_Omer_DEMIR | Full-Stack-Project | WEEK3 #446
Conversation
Mr-DEM1R
changed the title
| setVideos([...videos, videoToAdd]); | ||
| setNewVideo({ title: "", url: "" }); | ||
| setIdCounter(idCounter + 1); | ||
| } |
There was a problem hiding this comment.
It looks like your frontend doesn't try to talk to a server at all. Other than that, it looks like you have a pretty solid frontend and a pretty solid server which uses a database. Can you connect them together to talk to each other?
There was a problem hiding this comment.
I corrected and updated the code file.
client/src/component/AddVideo.jsx
Outdated
| }; | ||
|
|
||
| const getYouTubeVideoId = (url) => { | ||
| const videoIdMatch = url.match(/(?:\/|v=)([A-Za-z0-9_-]{11})(?=&|$)/); |
There was a problem hiding this comment.
What assumptions are you making in this code? When can you imagine those assumptions may be violated?
There was a problem hiding this comment.
The code assume that the input url follows a certain format of YouTube video URLs. Inside the URL, it expects a video ID containing either "/v=" or "/watch?v=" followed by an 11-character alphnumeric and underscore (A-Za-z0-9_-). This assumes the format is compatible with YouTube's video URLs.
If the input URL does not follow the standard YouTube URL format (for example, it is a shortened URL, a mobile URL, or contains custom parameters), the regular expression may not find the video ID correctly or at all.
| const [videos, setVideos] = useState(props.result); | ||
|
|
||
| function convertToEmbedUrl(url) { | ||
| const videoId = url.split("v=")[1]; |
There was a problem hiding this comment.
What assumptions are you making here? What would happen if those assumptions were violated?
There was a problem hiding this comment.
-The useState(props.result) statement assumes that props.result appears as the initial value of the state variable named videos. Using a series of videos.
- The convertToEmbedUrl function assumes that the input url is a YouTube URL in a specific format and tracks the video ID by containing the "v=" parameter in that URL.
-If the input url does not have the specific format of a YouTube URL containing a "v=" parameter and followed by a video ID, the split method may not find a match and return undefined, causing errors when trying to access videoId[1].
-If the input url is not a valid YouTube URL or is empty, the code may likewise generate errors or show unexpected behavior.
There was a problem hiding this comment.
Good analysis of the assumptions here!
One thing I'll draw your attention to is that if the URL doesn't contain v=, your app is likely to completely crash and hang. If you checked the length of the array before trying to get an element out of the array, you could more gracefully handle the problem.
server/.env
Outdated
| @@ -0,0 +1 @@ | |||
| DBConnLink = "postgres://project_data_eqrf_user:MldQZenHLTFNPhwOUA6Nx6a9zrKRMfXc@dpg-ck1o4bnhdsdc739osv00-a.oregon-postgres.render.com/project_data_eqrf"; | |||
There was a problem hiding this comment.
You probably didn't mean to check this file in - you should probably remove it from git, change the password, and add it to your .gitignore
server/DBConfig.js
Outdated
| const { Pool } = require("pg"); | ||
|
|
||
| const videosPool = new Pool({ | ||
| connectionString: |
There was a problem hiding this comment.
You probably didn't mean to check this in either - try to have this use your .env file rather than putting a copy of the connection string here.
client/src/component/AddVideo.jsx
Outdated
| const videoToAdd = { | ||
| ...newVideo, | ||
| rating: 0, | ||
| id: idCounter, |
There was a problem hiding this comment.
Ordinarily we'd expect the server to choose an ID, rather than the client, because a client can't know what IDs the server already knows about.
It looks like the server ignores this field completely, so it seems strange to set it. I think you can stop setting it, and also remove the idCounter state completely?
| if (newVideo.title && newVideo.url) { | ||
| const videoId = getYouTubeVideoId(newVideo.url); | ||
|
|
||
| if (videoId) { |
There was a problem hiding this comment.
This code totally works, but as a style thing, some people would write this as:
if (!videoId) {
alert("Invalid YouTube URL. Please enter a valid YouTube video URL.");
return;
}
const videoToAdd = ...
...There are three nice ideas here:
- Your code isn't as indented, so is a bit easier to read.
- Detecting what constitutes an error (no videoId) and what we do about it (alert, and don't do the rest of the function) is close together. Right now, when we get to the else, it's pretty easy to forget what the
ifcondition was, because it's so far away. - It allows us to mostly read the function as a straight line - we don't need to jump between lots of ifs and elses - reading the function top-to-bottom, it does things in order, but may stop early, rather than jumping around so much.
| <div> | ||
| <h2>Videos</h2> | ||
| <ul className="ShowingVideos"> | ||
| {videos.map((video) => ( |
There was a problem hiding this comment.
It would probably be more clear to extract a component for "showing one video", rather than having to work out what exactly is happening inside the map callback.
| const [videos, setVideos] = useState(props.result); | ||
|
|
||
| function convertToEmbedUrl(url) { | ||
| const videoId = url.split("v=")[1]; |
There was a problem hiding this comment.
Good analysis of the assumptions here!
One thing I'll draw your attention to is that if the URL doesn't contain v=, your app is likely to completely crash and hang. If you checked the length of the array before trying to get an element out of the array, you could more gracefully handle the problem.
|
Hello Daniel , I made the necessary arrangements. I hope I have completed the full stack project. Thank you for your detailed review. |
No description provided.