Not gonna lie, this is extremely valuable information for junior developers (and even more experienced ones), thank you for sharing your knowledge, and by the way, your explanations are very clear
I'm a junior dev, and I was wondering why you choose map as the state data structure instead of a set, because you're not keeping the state checked per id, you're only keeping the ones that are checked. And even if you kept all ids and just toggled between true or false, in this context it still makes more sense to me to use a set.
Yes you’re right. Iirc I changed the code to use a set after recording this video. You can check the blog post for that implementation. That’s the advantage of a blog post: you can’t just adjust the video but the blog is easy 🙂
Mapping is unnecessary all together and you can manage the entirety of this component using a single state. Anyone else also hate the fact that they don’t separate the styling from the implementation of the functional component? You can style elements based on the state of an input in css..
A thought on the data structure you chose. A map works well. Whenever I see a map where the value is a Boolean, I ask myself if a Set would work instead. In this case, I think what you really want is a Set.
I'm not a junior dev anymore, but thank you for this insightful video. Personally, when I am preoccupied with the development or maintenance of a project, I sometimes form some bad habits, like lack of proper error handling in some logical cases, because the project needs to be developed quickly and I slack off while chasing the fast paced development of features. These types of videos really bring me back into reality and make me realize some of my newly acquired deficiencies in React.
Nice! This was a good example of simplifying the code. I think it would be interesting to see how you might word a review on an example PR like this one.
Absolutely loved this article. I read it last night and could apply it in some work I went back to today, where I'd added unnecessary state that I could've derived
That's a good suggestion. Honestly I didn't think that much about the JSX. But yeah, especially the table row would be a good candidate for a separate component imo
For the first issue: Pasing an object as a parameter to fill can cause another major issue that all the object have the same reference. Here is an example: let arr = Array(5).fill({a:1,b:2}); arr[1].a = 6; //This will cause all a to be 6 for "all" the objects in the array
As a beginner it's easier to make the mistake... Not a lot of tutorial drive home the concept of derived state well. So as a a beginner you tend to default to patterns you are aware of.
I gotta say.. your tone and presentation is super monotonous, but the value of what you say and explain is great. Well done I agree with all you said. One thing about the Map though. Personally I dislike using Maps on very big data arrays, and simply revert to arrays + writing helper functions for search and filtering but a Map is much more readable and "easier" to use
Hello, as your map of completed issues only ever has a value of true, was the only reason you used a map instead of an array/set here so the lookup could be faster when checking if the issue is completed or not? A set of IDs seems to make more sense imo Thanks
AFAIK looking up in a map has a constant access time due to it being stored as a hashtable behind the scenes. Meaning it will be the same access time no matte how many elements will be in that list.
@@MichaelKireYou are right, but so do sets. Essentially a set is a map mapping to booleans. But I'm not a javascript guy so I also don't know why a map was preferred over a set here.
Scrolled a little more, turns out there is no good reason and he ended up changing to a map and updating the blog post but didn't redo the video (wouldn't expect him to)
Thank you so much. If possible, please upload a video on how to develop a production ready react app, for example the folder structure, custom hooks, global context store, global axios confg, etc.
4 หลายเดือนก่อน
Great video, really helpful for someone getting started!
wow, great work! how long would it take you to do these refators in real time? I imagine its not done typically within 20 minutes... thanks for the video
@@bensonding1703 I migrated the blog post to using a set after realizing this. Unfortunately you can’t just change the video that easily 😬 But you’re completely right of course
I encountered this very same problem (diff example but same premise) and I tried to use a Set with redux toolkit, didn't want to work for some reasonso I had to resort to a dictionary/json where the keys mapped to any value (1, true, null, didn't matter). Then I replaced redux with zustand and it let me... how odd.
array is way faster then a map in this example. only in huge list it would make a difference that's not common to load all on the client side. a small array like this is even faster if you have to loop through it a few times then the overhead of having function for the map. but dont worry a lot of junior devs make that mistake
Very good. I just don’t agree with ternaries being simpler that if statements. I know frontend developers tend to run from ifs as the devil does from the cross but I think ternaries are a common source of confusion and bugs in lots of front end code. Good job
Oh I really don’t like ternaries actually. I try to get around them as much as possible except for really simple conditions. Not sure what I said in that video though 😂
This was very insightful. As a junior dev I learned a lot. Most importantly is that I really have to start using the new Map instead of just objects and I actually didn't know about the ".size" property. does it work with "regular" objects? Last question is a bit longer but since you touched on that subject I'll allow myself to ask that: I have an app in which I have a sidebar with buttons which run functions (ofc) and one of those uses the text in the main layout. In that case, using a ref felt like bad practice since I would have to either forward it through many cmps, store it in the global store or use context instead of just accessing it directly... what is your take on that? Thanks! :)
Thanks a lot for the feedback. You only have the size on Map and Set not on regular objects. Regarding the shared text: not sure but this might be a good case for using context. Maybe there's also an alternative way of structuring the app that's more efficient. Hard to say. If you want me to do a similar review feel free to send me the code at review@profy.dev
@@profydev Thanks for the comment (I subbed by the way, keep it up, I know many people are looking for content exactly like that). I wish I could, that would be very useful. but it's quite a big app which I wrote for work and it's on a closed network :( Can I email you the question with more context anyway?
@@Tom_Rose Sure, feel free to send me an email to johannes@profy.dev. There's no need to share the whole code btw. A reduced version on CodeSandbox would even be better
As a senior swe, one thing I would nitpick is this awful double negation (!!). I would propose changing it to the Boolean constructor and adding a custom eslint rule disallowing UnaryExpression (!) as an argument of UnaryExpression (!)
Thanks, good catch. It's even better to replace the Map with a Set (suggestion from someone on Reddit). I adjusted the code in the blog post (see the description). But unfortunately, I can't update the video anymore
Instead of using a Map maybe just have an array of stateId's and use indexOf or includes instead of get on the Map its both faster and simpler because the truthiness is defined if it exist and dosn't carry a true value as well so its both faster and uses less memory
I understand the convention that methods returning booleans should begin with `is`, since the assumption is that the method's object is the noun part of the clause. However, free variables are also nouns, not actions. I cannot help but feel that this convention being carried over to variables is a consequence of dogma, rather than usefulness. "IsIssueOpen" is a question, not a statement. "If is issue open" is not good English. Does anyone have an argument to counter this?
im confused about the usage of !!, i understand it does a double negation and converts to boolen, but why concert to boolean if the value is falsy anyway?
One question , the thing where you replace the for loop for counting elements that have the status open with a filtered array ( arr.filter().length ) just to have it just "pretty" impact the memory usage ? U don't just iterate thought the the array that you have , u create a new array so , allocating more memory .Maybe a middle ground can be a reducer :D
Can you add a live clock extension to vsCode so we can judge how long the rafactor actually took otherwise imposter syndrome starts acting up with all the time skips :D
The double exclamation operator is to forcibly cast a boolean value in a variable. let something = "Test"; console.log(typeof something); // string let otherthing = !!something; console.log(typeof otherthing); // boolean So basically, the !! in the disabled attribute forces the assigned variable to act as a boolean value. It's for type safety.
This video is heavily edited. So all the thinking breaks are cut out and the typing is sped up. No worries, you're probably not too slow :) If you want to watch me in real time check out this playlist. Takes me 10 hours to build a freaking sidebar navigation (plus a lot of setup and stuff but still): th-cam.com/play/PLo6qcHP9e9W5T0cwCWsQ4qcoXATqvMzcS.html
2:45 - I disagree about using refs to access the dom in 2024. Ref access was Reacts way of hiding their performance issues back in the day. But dom access is fine now. 10 years later good grief.
Very helpful review. Thanks for sharing Although I'm still not able to understand one thing In the first line of handleOnChange method you doing this const updatedCheckedById = new Set(checkedById); (I have taken this from your blogpost , in the video it is Map) How come this update the previous Set and if so the next if check should return true, but surprisingly it is returning false Can you share some thought here Apologise for my lack of knowledge in advanced
I'm not sure if I get the problem right. But let me try to explain: 1. Initially checkedById is an empty set 2. new Set(checkedById) just clones the current set. The goal is to create a new reference so we can use it to update the state (as React state has to be immutable) 3. The first if uses updatedCheckedById.has(id). Since checkedById is initially empty so is updatedCheckedById. So .has(id) returns false. 4. If an item is already selected its id is included in the set. Then .has(id) returns true and the id is removed from the set. The result is that the item is unselected. Does that make sense?
Thanks for the feedback. True, somebody on Reddit pointed out the same thing. The blog post that's linked in the description now uses a Set. Unfortunately I can't update the video here.
100% do more refactoring videos, these are rare and idk if it's specifically your unique idea to do this, but this is actually gold content
Not gonna lie, this is extremely valuable information for junior developers (and even more experienced ones), thank you for sharing your knowledge, and by the way, your explanations are very clear
Nicely done!
As a senior developer I can assure that your work here is a gem.
Thanks, that means a lot
I'm a junior dev, and I was wondering why you choose map as the state data structure instead of a set, because you're not keeping the state checked per id, you're only keeping the ones that are checked. And even if you kept all ids and just toggled between true or false, in this context it still makes more sense to me to use a set.
Yes you’re right. Iirc I changed the code to use a set after recording this video. You can check the blog post for that implementation. That’s the advantage of a blog post: you can’t just adjust the video but the blog is easy 🙂
Mapping is unnecessary all together and you can manage the entirety of this component using a single state. Anyone else also hate the fact that they don’t separate the styling from the implementation of the functional component? You can style elements based on the state of an input in css..
A thought on the data structure you chose. A map works well. Whenever I see a map where the value is a Boolean, I ask myself if a Set would work instead. In this case, I think what you really want is a Set.
Never mind I see you addressed this in another comment. I didn’t see that until now! Thanks
I'm not a junior dev anymore, but thank you for this insightful video.
Personally, when I am preoccupied with the development or maintenance of a project, I sometimes form some bad habits, like lack of proper error handling in some logical cases, because the project needs to be developed quickly and I slack off while chasing the fast paced development of features. These types of videos really bring me back into reality and make me realize some of my newly acquired deficiencies in React.
Such a well-made video. Kudos to Profy,, this channel deserves way more subscribers and views!
Thanks for the nice words
Nice! This was a good example of simplifying the code. I think it would be interesting to see how you might word a review on an example PR like this one.
Absolutely loved this article. I read it last night and could apply it in some work I went back to today, where I'd added unnecessary state that I could've derived
Thanks so much for the feedback!
Thanks for sharing. Cool work.
One thing I could suggest is to split component into several smaller once.
That's a good suggestion. Honestly I didn't think that much about the JSX. But yeah, especially the table row would be a good candidate for a separate component imo
good video! I would make a habit of saying "simply" less often. what's simple and obvious to you might not be to others!
For the first issue: Pasing an object as a parameter to fill can cause another major issue that all the object have the same reference.
Here is an example:
let arr = Array(5).fill({a:1,b:2});
arr[1].a = 6;
//This will cause all a to be 6 for "all" the objects in the array
Great stuff! I've definitely been sleeping on the usefulness of maps! Subscribed :)
Very good!
Yes, I want to see more of this stuff.
What an amazing video for someone who is learning react. Thank you for this!
Juniors code is better than some of the seniors where I work.
I wish I saw this YEARS ago, that was awesome and insightful
excellent i see a lot of general concepts i can apply, especially when implementing that none selected box, thank you
WE NEED MORE OF THIS CONTENT
This is excellent! getting back into React after a long break, and your videos are a godsent. Keep it up!
Thanks :)
Last issue is not using typescript
Thanks. As a just learned react developer, this will help me
Awesome. Would like to see more videos like this
this was amazing and strangely therapeutic
Unnecessary state variables are the worst.
As a beginner it's easier to make the mistake... Not a lot of tutorial drive home the concept of derived state well. So as a a beginner you tend to default to patterns you are aware of.
nah, unecessary useEffect is the worst hahaha
Hi, hope you will have more of this video series, or refactoring playlist. Thanks for the content! thumbs up!!
Thanks for the feedback. Check my React Architecture playlist. That's basically a larger refactoring as well only with a focus on architecture
I gotta say.. your tone and presentation is super monotonous,
but the value of what you say and explain is great. Well done
I agree with all you said. One thing about the Map though. Personally I dislike using Maps on very big data arrays,
and simply revert to arrays + writing helper functions for search and filtering but a Map is much more readable and "easier" to use
Great work brother . Expecting more like this 👍👍
Hello, as your map of completed issues only ever has a value of true, was the only reason you used a map instead of an array/set here so the lookup could be faster when checking if the issue is completed or not?
A set of IDs seems to make more sense imo
Thanks
AFAIK looking up in a map has a constant access time due to it being stored as a hashtable behind the scenes. Meaning it will be the same access time no matte how many elements will be in that list.
@@MichaelKireYou are right, but so do sets. Essentially a set is a map mapping to booleans. But I'm not a javascript guy so I also don't know why a map was preferred over a set here.
Scrolled a little more, turns out there is no good reason and he ended up changing to a map and updating the blog post but didn't redo the video (wouldn't expect him to)
Wow this video was actually amazing, that's wild!
Awaiting more video like this, Thanks for posting!!
The refactoring is just on point. Good work.
aah I could watch these all day. Loving it! Very tempted to send my code... hoy could I do that? Cheers!
This Is really helpful as a junior dev myself!
This is was amazing, would love to see more content like this
Do more junior refactor videos!!❤
This is wonderful, this kind of content is very helpful
I love these kinds of videos! Great stuff!
Keep doing these awesome videos. I find them so helpful!
Great video! I would love to see more of these
Nice! Would be interesting to see how the solution can be extended for a filterable list
Subscribed! I want more of this. 🤩
Thank you so much. If possible, please upload a video on how to develop a production ready react app, for example the folder structure, custom hooks, global context store, global axios confg, etc.
Great video, really helpful for someone getting started!
thanks for this, helps a lot as a junior
Damn, imagine watching these videos, joining a company as junior dev and writing such code what he wrote after refactoring.
very nice, clear and helpful for me as im practicing with react! although i needed to watch it in 0.75 but thats because im not native english 😅
Awesome stuff, would definitely love to see more of this. Liked 'n subscribed!
please do more refectoring session
Great video Johannes! I had your blog on this lined up for reading for a year now. I saw the video and just went for it now. 😆
Great video!
Loved this kind of video!
I subbed, hoping to see more refactoring videos!!
Excellent refactor, well done
wow, great work! how long would it take you to do these refators in real time? I imagine its not done typically within 20 minutes... thanks for the video
I wonder why using a Map instead of a Set to store checkedByIds
, since it’s just a list of unique strings?
@@bensonding1703 I migrated the blog post to using a set after realizing this. Unfortunately you can’t just change the video that easily 😬 But you’re completely right of course
@@profydev Got it. Thank you! really nice video
I encountered this very same problem (diff example but same premise) and I tried to use a Set with redux toolkit, didn't want to work for some reasonso I had to resort to a dictionary/json where the keys mapped to any value (1, true, null, didn't matter). Then I replaced redux with zustand and it let me... how odd.
array is way faster then a map in this example. only in huge list it would make a difference that's not common to load all on the client side. a small array like this is even faster if you have to loop through it a few times then the overhead of having function for the map. but dont worry a lot of junior devs make that mistake
I would love to see more
Thank you for this video! Helpful 💓💓
Really cool video! Thanks!
Very good. I just don’t agree with ternaries being simpler that if statements. I know frontend developers tend to run from ifs as the devil does from the cross but I think ternaries are a common source of confusion and bugs in lots of front end code.
Good job
Oh I really don’t like ternaries actually. I try to get around them as much as possible except for really simple conditions. Not sure what I said in that video though 😂
more videos like this please!
Amazing video ! May I ask when is it appropriate to use a map vs an array?
thank you very much for the video
Great video :)
This was very insightful. As a junior dev I learned a lot. Most importantly is that I really have to start using the new Map instead of just objects and I actually didn't know about the ".size" property. does it work with "regular" objects?
Last question is a bit longer but since you touched on that subject I'll allow myself to ask that:
I have an app in which I have a sidebar with buttons which run functions (ofc) and one of those uses the text in the main layout. In that case, using a ref felt like bad practice since I would have to either forward it through many cmps, store it in the global store or use context instead of just accessing it directly... what is your take on that? Thanks! :)
Thanks a lot for the feedback. You only have the size on Map and Set not on regular objects.
Regarding the shared text: not sure but this might be a good case for using context. Maybe there's also an alternative way of structuring the app that's more efficient. Hard to say. If you want me to do a similar review feel free to send me the code at review@profy.dev
@@profydev Thanks for the comment (I subbed by the way, keep it up, I know many people are looking for content exactly like that).
I wish I could, that would be very useful. but it's quite a big app which I wrote for work and it's on a closed network :( Can I email you the question with more context anyway?
@@Tom_Rose Sure, feel free to send me an email to johannes@profy.dev. There's no need to share the whole code btw. A reduced version on CodeSandbox would even be better
As a senior swe, one thing I would nitpick is this awful double negation (!!). I would propose changing it to the Boolean constructor and adding a custom eslint rule disallowing UnaryExpression (!) as an argument of UnaryExpression (!)
Eslint rule is way too overkill but I do agree on the double negation😊
has-method fits here better 5:45
Thanks, good catch. It's even better to replace the Map with a Set (suggestion from someone on Reddit). I adjusted the code in the blog post (see the description). But unfortunately, I can't update the video anymore
Instead of using a Map maybe just have an array of stateId's and use indexOf or includes instead of get on the Map its both faster and simpler because the truthiness is defined if it exist and dosn't carry a true value as well so its both faster and uses less memory
Do it just do it this is awesome
I understand the convention that methods returning booleans should begin with `is`, since the assumption is that the method's object is the noun part of the clause. However, free variables are also nouns, not actions. I cannot help but feel that this convention being carried over to variables is a consequence of dogma, rather than usefulness. "IsIssueOpen" is a question, not a statement. "If is issue open" is not good English.
Does anyone have an argument to counter this?
wow, more of this
im confused about the usage of !!, i understand it does a double negation and converts to boolen, but why concert to boolean if the value is falsy anyway?
More than that, couldn't he just have used Map.has() instead of double negating Map.get()?
One question , the thing where you replace the for loop for counting elements that have the status open with a filtered array ( arr.filter().length ) just to have it just "pretty" impact the memory usage ? U don't just iterate thought the the array that you have , u create a new array so , allocating more memory .Maybe a middle ground can be a reducer :D
Nvm , I didn't get to the part where you mentioned about memory :D , great job XD
Haha thanks
Can you add a live clock extension to vsCode so we can judge how long the rafactor actually took otherwise imposter syndrome starts acting up with all the time skips :D
Haha that wouldn't help either as this was for sure not the first time I refactored this code :)
Can you point me to resources explaining the !! on the input's disabled attribute?
The double exclamation operator is to forcibly cast a boolean value in a variable.
let something = "Test";
console.log(typeof something); // string
let otherthing = !!something;
console.log(typeof otherthing); // boolean
So basically, the !! in the disabled attribute forces the assigned variable to act as a boolean value. It's for type safety.
Omg u do it all so fast compared to me, it makes me cry🥺😭
But i wont give up 💪
This video is heavily edited. So all the thinking breaks are cut out and the typing is sped up. No worries, you're probably not too slow :) If you want to watch me in real time check out this playlist. Takes me 10 hours to build a freaking sidebar navigation (plus a lot of setup and stuff but still): th-cam.com/play/PLo6qcHP9e9W5T0cwCWsQ4qcoXATqvMzcS.html
@@profydev i watch it in half speed haha, thanks!
Good communication, u are awesome
2:45 - I disagree about using refs to access the dom in 2024. Ref access was Reacts way of hiding their performance issues back in the day. But dom access is fine now. 10 years later good grief.
Very helpful review. Thanks for sharing
Although I'm still not able to understand one thing
In the first line of handleOnChange method you doing this const updatedCheckedById = new Set(checkedById); (I have taken this from your blogpost , in the video it is Map)
How come this update the previous Set and if so the next if check should return true, but surprisingly it is returning false
Can you share some thought here
Apologise for my lack of knowledge in advanced
I'm not sure if I get the problem right. But let me try to explain:
1. Initially checkedById is an empty set
2. new Set(checkedById) just clones the current set. The goal is to create a new reference so we can use it to update the state (as React state has to be immutable)
3. The first if uses updatedCheckedById.has(id). Since checkedById is initially empty so is updatedCheckedById. So .has(id) returns false.
4. If an item is already selected its id is included in the set. Then .has(id) returns true and the id is removed from the set. The result is that the item is unselected.
Does that make sense?
Can i use Map, Set, Proxy, inside a useState Hook?
You can definitely use Map and Set with useState. Proxy I'm not sure
Boss level♥
How did he delete the whole line at once??
Probably ctrl x without selecting anything. Then the current line should be gone.
You people need to use the vim extension. If you think deleting a single line is impressive, vim will blow your fkn mind
Could have used a set
Thanks for the feedback. True, somebody on Reddit pointed out the same thing. The blog post that's linked in the description now uses a Set. Unfortunately I can't update the video here.
Hello MrBeast!
10:07 idk much about react, but readability was easier. Now Id make a comment above about the false states.
Interesting
bitflag, where?
why doesn't this guy blink
Very cool content but a bit too fast IMHO. 👍
I feel bad I just don’t understand
Mmmore of. This please
Those are some long variable and function names lol
Wow Sar, how to become pero laik u
WTF is with the bot comments?
Love it, thanks from sharing.
Thanks for watching!