Live refactoring a subscriber's React code
ฝัง
- เผยแพร่เมื่อ 6 ก.ย. 2024
- A subscriber gave me permission to clone his repo and refactor his code. Join my while I try to understand his code and clean it up.
------------
🤑 Patreon / webdevjunkie
🔔 Newsletter eepurl.com/hnderP
💬 Discord / discord
📁. GitHub github.com/cod...
To anyone watching this, my approach to keeping track of the page in state and breaking the Hateoas url returned from the api is BAD. I also didn’t have time to find ways to cache these requests which would help with performance. My main focus was cleaning up the code in the video. I still think there were some interesting things discussed in this video, but maybe don’t ignore the urls returned from the api in the future. They are there for a reason. 🍻
damn bro you are cool!
Thought process was great to hear / understand despite refactoring not being *the most* optimal
May I ask how I can be lucky enough to send a project to you to refactoring?
Hi, may I ask how you would've kept the page state on second thought? Would you have parsed the next / previous page url returned by the people request to return it and then use it to update a nextPageNumber / url value?
The only issue I see with the way you kept it is we may possibly run out of pages without detecting it beforehand, or maybe we could encounter problems if the page numbers are not sequential.
This project seemed more suited for classic serverside rendering than react. The oldschool pagination especially. No clientside js was really needed. If this was serverside you could cache the entire html response for each page for a long time and get a lightning fast render, instead of every visitor having to call the api.
You'd also get simpler url based navigation and make the content crawlable of the box.
Using React seemed to complicate things by requiring page to be kept as state, and causing issues with duplicate requests on mount etc.
But of course it’s probably intended as a simple starter demo to learn React and could make use of React later for filtering etc.
awesome to see this happening live. I don't think many folks are doing this style of video on TH-cam, so a great niche to carve out, and your commentary is valuable in understanding decision-making. Keep it up!
thank you for this super insightful, thinking out loud, and showing the real struggle of programming and refactoring stuff, can you do more of this?
I feel like something that would’ve improved the speed a lot would’ve been to create a species map and a homeworld map, so that you don’t need to request multiple times the same data
Great idea!
yea, and we get all the unique "id" and store it in the array, then we just reference it back for current and next page onwards, tho, would be an issue for live data that kept updating
that's why they created React Query.
great video
and also, for those like me wondering name of extension that shows github record inline and error/warning description
they are : git lens + error lens
This is just awesome! It's really great to show how to write/clean up code in a session like this than teaching some basics which can be found in 100 different places. Kudos.
this was super interesting to watch, and observing your thought process through certain scenarios led me to learn some cool things. also, you are insanely quick with the keyboard shortcuts, i wish i was that fluid 😅
Superb Livestream cody. You're sharing your thought process and then doing.
Valuable content. More streams needed sir
I appreciate that!
Really helpful to watch you work through this. Thank you.
As a begginer this is one of the best vidoes ever. thank you so much chief i hope you all the best < 3
Would love to see more like this, different project types or other frameworks, adding linting or tests, maybe even some pure vanilla or converting a project from js to ts, etc. There's tons of videos starting things from scratch out there but not a lot for refactoring and or reworking existing projects.
Yeah the problem is finding code that’s in a bad enough state that makes it worth refactoring. People have sent me code but their structure is “good enough” that I don’t think it warrants a video.
@@WebDevCody Maybe looking through some other TH-camrs really old projects that aren't up to today's standards anymore, especially pre-es6 stuff, would be a good source. CodingGarden, CodingTrain, etc. It would be a nice follow-on from their content since you'll have their original video making the project from scratch so you don't have to guess the intention, and most of them put the MIT license on all their stuff so it can freely be used.
@@WebDevCody is this a challenge? 😉
@@cody7855 send me some bad react codr
Bro..That is just awesome .
I love it every minute of this video, as a junior developer here in brazil, i learned a lot. Thanks
U should make more of this.
Thanks again dude.
This was super helpful. I'm currently a FE Eng with about 1.5 YEO and it's really great to see your thought process and pick up some tips along the way. Would like to see more videos like this!
Is there any place where you describe your editor setup for things like the inline linter messages?
Great walkthrough how to think when setting up project with clean and easy readable code. I particularly enjoyed the thought process and your iteration of improvements in the code as you made new discoveries in the code base.
This was really very educative and very very appreciated
We definitely need more stuff like this
I would like to see more of this. I think is very interesting to see you going through the code review and refactoring process. Also explaining the train of thought is very good.
I dig this. Interesting to follow along with someone else's thought process, and you make it very clear to see what you're doing.
The bad thing about many APIs is that you can't JOIN (character -> homeworld, ...). That why GraphQL is a good idea. If you have such an API it is a good idea to first fetch all homeworld data, put is in a hashmap (URL in this case as key, as mentioned better ID) and then when fetching people lookup the homeworld data.
Yeah using this api has made me understand why graphql is a thing.
Love these refactoring vids. So much more insightful then any tutorial
i had a good quick dinner watching this.was quite entertaining. thanks
Looking at the requests for the homeworlds they seem to be fetching a lot of the same homeworlds. So one improvement would be to loop through all the people beforehand and extract unique homeworlds into a Set. Probably the same for species.
And then only do fetches on these unique URLs and map them to the people.
And this would probably reduce the amount of API calls by a lot.
Very cool idea to do this ❤
I think refactoring, especially for scalability, is greatly overlooked in TH-cam tutorials. In reality, what is important is not only what works, but what will work as a project grows.
I don't even work with front-end, still, it was nice to see this content.
So the first thing you did was break the API that returns the previous/next page parameter because "reasons". It's a very common practice to have the API being "discoverable" (HAL JSON is an example of this) meaning that the API itself will tell you what options you have, and by doing that, you don't need to handle that state yourself, the API will tell you. Secondly, and I haven't watch through all of this to verify, but going to next page with your solution will get out of bounds when there are no pages left. Cleaning up code serves no purpose if the solution doesn't work properly.
Yeah I agree I messed some stuff up
Well the bright side is that this content is very good for your channel. If you manage to add the shortcuts and some other points fellow dudes said I would watch again anytime. But again this video helped me thanks
I just started learning js and it was nice to understand what you were saying. I’m only 15 hours into the basics. I enjoyed this video seeing was proficiency looks like!
You mention using prettier and call it a linter. First thing anyone should add is ESLint (a linter) and pair it with prettier (formatter). Both have their value and both should be used. ES List would have fixed a lot of the code smells there.
Guess eslint is there but looks poorly configured or something. Cant really tell
Another tip. With application code you want to run `npm ci` and not `npm install` to respect their package.json file.
Good tips
I enjoy the video.
I would like to see more refactoring videos from you.
It'd be great if you had your keystrokes appear on screen! I'm just watching you do all these vscode shortcuts and wanting to use them myself, but I have no idea what you're doing 😢
I try to find one
earned my sub bro, I love this kinda content
keep up the good work.
I hope you will add more videos like this because it's very helpful.
thank you very much
This was focused pretty heavily on jumping to naming convention instead of taking time to read and understand his intention before refactoring it. I get that it's like a live sort of thing and you have an authority over the code but it ended up costing extra time. Nice videos and channel btw
yeah for sure, I'll try to slow it down next time. Thanks for the feedback
But you see, the responsibility to make things readable and the intention clear, falls with the developer writing the code. The point here is you should make it easier for other people to understand you, and your intentions. One of this is using a convention, another is to make the implicit, explicit as much as possible.
@@ponderatulify you're absolutely right about that, and I don't mean to dismiss that effort at all. My preference tends to prioritize understanding thoroughly the logic then handling updating the readability when it comes to refactoring others' code. But again, I do 100% agree that readability is very important for the longevity of the code and understand that others flows differ
I kinda agree, was scary to see homeworld being refactored before understanding what it was used for.
But in this case, since there was an initial bug preventing data from rendering I can understand this approach.
And for some of us - just reading through code doesn't quite make it stick and it's not until we start working with it that we get engaged and really grasp what's going on. While doing code reviews, even if the code is perfect I sometimes start messing around with it just to really get my brain going
Good job with the video! Always interesting to see some junior code and some refactoring :D
I did find the video a bit hard to watch, maybe because you dove straight into renaming/ moving code, before we had any idea what the app did. I think you also didnt seem confident in many of the refactors because you hadnt understood the app too.
Maybe in the next video, spend a bit of time explaining the code and adding some comments of what you like/ dislike and how you intend to refactor it.
But I think the fact that you found it hard to understand initially means its a great example repo to refactor!
Also, for the performance, they chose to load everying before displaying it on page. I would question their decision on this and ask if they could display the base character in the table and load the homeworld/ species later? It might be better UX.
Thanks again for the video!
For sure, I often jump into code instead of taking a minute to understand it first. I’ll take notes for any future refactoring videos. The reason I jumped into naming is because good naming helps with understanding the code base
The API is poorly setup for this kind of app. I do believe it makes more sense to click on something to show homeworld and such however. I'm unsure if it would be better UX however. It makes more sense to me if I can click the world name rather than an icon for example.
Loved the video! Maybe you could share the “plugins” that u use in VSC?
never seen your stuff before but this was awesome to watch
That moment of: “The Star Wars API is crazy!” 🤣
I think this was one of the best code refactoring video on TH-cam 🔥🔥🔥
I loved your refactor, everything looks so clean 🔥
23:48 I feel like this was the golden spot and the moment I would personally leave the code at if I were you. Well, except for the homeworld homeWorld thing of course.
👍 a senior video is fantastic, we learn a lot :)
Amazing video, please bring more like these.
I have prettier installed and followed tutorials for setup. It was working good for a bit, but when I started learning HTML and CSS it stopped working.
There’s an error when I click on prettier but I do not remember it off the top of my head!
Any Ideas on what it could be?
In react 18, useEffect runs twice btw
Great vid, I learned a few things hehe, as I'm still "new" to React.
What's the name of extension that gives you the compiler like error/warning messages?
I didn’t know about the self executing async function in useEffect, nice trick. I always define an async function and call it manually, it’s still janky but I like this better.
Great tips for beginners - clean code is huge
Is it better to use a callback when set methods are dependent on their own current value? setPage(prev => Math.max(1, prev - 1)) ?
By the way thanks for sharing your experience refactoring codes. its always challenging to understand what the other persons want to do.
cheers mate.
If you want to be safe, sure.
Love this video. Keep it up!
@WebDevCody kindly tell me the extension name which is compiling in every line of code
what is the name of the extension to see inline error suggest in your video?
Thank you for makinng these live code refactors.🤝
Is it better to use below pattern for states which depends on prev state value.?(whats your opinion)
__________________________
setPage((prev)=> prev +1)
_________________________
Usually you have to use the state callback in certain situations.
it is a better approach as it reduces function re-initialization and re-renders
i just wanted to comment something very similar, yes this is the best practice approach not only for the reason Joseph Chong pointed out, but also because it is more save.
in this particular example it does not *really* matter, but in other examples the state of page might not yet be updated from a previous call to setPage (for example if you would implement setPage(page +1) two times after each other to go forward two pages, it would only work with the callback approach), basically any time your setState call depends on the previous state you should use the callback overload.
I’m only about 6min in so maybe it gets covered later, but I want to point out that you should abstract your data fetching to a custom hook that utilizes a useEffect (in 2022, could change later). More specifically, you should always use swr or React Query (recently renamed TanStack Query), even for small projects like this. Train the brain to always reach for it.
Yeah, I usually just use react-query for my side projects
It's very interesting and enjoyable (even if I'm backend dev).
Hey, whats the extension that displays the errors and warning next to the lines? That looks so helpful
i also would like to know this
@@joshuaebhoria8046 he liked it but never answered lol. But i found it myself. It's called 'error lens'
Its umbelieveble typing speed, I think Its x2
hi there, if it not much of a ask - what theme / plugins are u using? TY
Would it be more correct to use
setPage((prevPage) => ({
Math.max(1,prevState - 1)
}))
Compared to
setPage(Max.max(1, page -1)
?
Sure
Awesome video.
I would like to see more like this. Do you know where can I find some code to refactor?
It's an excellent exercise.
You’d have to find beginners and ask them for code
You would have really helped a new starter in the team with what you've just done. Looks good for what it's worth
This is very informative.. thank you.
What is the extension you have that shows you errors in code?
Error lens
@@kasemsanchompuwised1811 oh, thank you
oh thanks for your explanation. And what's theme you use?
this video awsome! learned alot.
Around 8:39
Why don't you just declare the function outside as separate function and call it inside useEffect. In that way not only you will avoid that self executing function but also that function can be called somewhere else as well
Yeah that’s probably an ok idea as well. If the function isn’t really reused anywhere else I don’t think it’s worth pulling out of the useEffect
@@WebDevCody sounds reasonable.
I am saying so because sometimes you want to do a couple of functions inside useEffect and it makes useEffect quite long and messier. So it's helpful to declare all those functions outside and call them inside useEffect .... So it will be like one function call per line
Being honest if i ever had to refactor this code I would have wrote it all again myself rather than go through the head ache of understanding and assuming what the other developer might have done lol
Which extension are you using that shows you errors inline?
Error lens probably
I think the returning links thing is a weird implementation of HATEOAS
Yeah you’re right, I’d honestly never worked with an api using hateos before.
I never really realized that a person doesn't have to have the best self-explanatory code possible when working at a real company. The best you can do to get the job done seems like it is just fine based on what I've seen in this video. There are some things in the code of this video from the discord user that are some obvious don'ts but you know it seems like you don't need to have all the answers.
I’m not sure I fully understand. Are you saying you don’t need to be great at self-explanation to work at a company, or are you saying I don’t have the best self-explanation? Anyway, I hope the refactoring was useful to watch
@@WebDevCody Oh, sorry about that. I edited the comment, its been a long day coding haha. Apologies
rest apis should actually by definition contain some redirect links, maybe thats the homeworld url's role
It is, I screwed up in this refactor
Come on, screwed up is a bit harsh. This isnt common knowledge, I had to read the whole wikipedia article about REST to actually become aware of it lol
@@miki_the_little198 you think I’d have seen an api like this before, but I guess I don’t mess with many third party apis lol
Great video .But what is the point of using axios in 2022?
Just preference. Fetch works fine, so does axios.
A little bit more understanding of the domain (star wars) would easily give away that you miss: homeworld caching and species caching. Many persons can have the same homeworld/species.
What is also not covered in this video: how would you feel if this was your API? this is doing so many api calls, but this point is covered by many other comments. For instance, BenRangel makes a good point. Nice video
bro I need one help I am not able to communicate between extension to react app which is opened on current window any suggestions guys anyone ?
Can someone tell me what the extensions is for showing errors such as Homeworld is assigned a value at 2:19
either eslint, typescript, or errorlens
@@WebDevCody thanks it was errorlens for the inline error display!
what is the extension that is telling you whenever something is not used or you have to change == to ===?
Probably eslintw
@@WebDevCody somebody said it was error lens.
I'm late to the party but instead of using an anonymous function inside a useEffect have it call a useCallaback function. You can then use async functionality just fine and add any additional logic there.
Would you mind sharing me a snippet of this idea? I'm curious how that would come out
I wouldn't extract the logic from the useEffect to its own function, as it makes it harder to follow through.
I think it’s fine
@@WebDevCody What about converting all the code into a custom hook for quick followup and low friction?
@@berakoc8556 that might help a bit. I think I should have abstracted the entire getPeople into its own function and done the promise all logic in there. The component shouldn’t know about how to fetch nested data
@@WebDevCody Extracting to it's own function is fine imo. Though (and this only is a minor issue) when declaring functions which don't rely on state such as that one, they should be put outside the component as otherwise the function gets re-declared every re-render. Think the rest is well done within the scope of the video. You're copping an awful lot of flak in the comments for really tangential stuff like SSR haha, why don't you optimise for SEO and CSS as well lmao. Hope you ignore them, might be easier to state your goals for the refactor in future vids but I like the format.
Neat video. Well done.
Hi, what theme do you use?
Maybe Community Material Theme High Contrast
To improve the code further there should be some caching going on. Right now it fetches the same data multiple times which is obviously worse performance than necessary and puts unnecessary strain on the network.
I'd say your refactoring was a great improvement overall. One thing I'm not a huge fan of however is having functions inside of the components. I believe everything that can be extracted outside of a component should be. No need to add potentially multiple instances of the same functions.
@Cottidae Would you extract the event handlers as well? What about utility functions that are specific to component? Also, where would you move these to, i.e. would you keep them in the same module or move them elsewhere, such as to a "utility" module?
@@nathanngo3628 I would keep them in the module if they aren't generic enough, so in 99.99% of cases they just go outside of the actual function for the component.
One of the reasons why is that if you write the functions inside your component, they are forced to re-render on update as well. So you can have loads of functions essentially be re-rendered constantly.
The useCallback hook doesn't help either as you'd just send a re-defined function to it every time. It would work properly if defined outside of the component however.
Usually not a performance issue, but in the case where you have a lot of small repeated components with logic, it can start to become noticable on lower end systems. So it's generally just easier to avoid it by staying consistent.
In many cases it's a matter of preferences however, but I do suggest you try this and see if it works for you.
@@CottidaeSEA Awesome, thanks for clarifying! I've been tossing up about whether I should have non-generic utility functions inside or outside the component, and I always assumed it was a matter of style, but this definitely gives me a good reason to keep them outside of the component.
This is why react query or swr is recommended
Yes I typically use react query, although I’m not sure the complexity of this code was due to the lack of react query, but more of the api doesn’t return the nested data associated with homeworlds or species, so a map and promise all was required that just made things confusing a bit
Do you cover Svelte as well?
Not really
How do you show the eslint error in the line of code?
Error lens
I don’t think the API data structure is unusual. It's poor that homeworld isn't named something clearer like homeworldUri. But I assume homeworld and species are separate DB tables and it seems super common to have APIs that basically just return uris to any data that's in a separate table.
Not saying it's good. I hate it. I feel like it's a lazy api design that's "all or nothing" instead of considering what the majority of API consumers need. It seems quite an easy choice to include homeworld name into the Person response. And would've save this project having to do dozens of extra requests.
Yeah I recently learned this is a HATEOAS api structure and normal for some rest apis. I don’t find it intuitive but I could see the benefits
@@WebDevCody yeah, was about to mention HATEOAS. While the fans of that principle seem like the worst offenders when it comes to ”incomplete data” and requiring multiple api calls, I do think this just counts as plain REST and often the idea is just ”if we included all reference table data it could be too much so lets exclude all”.
I think technically HATEOS is more about a structure for the links (perhaps they would’ve used links.homeworld instead of just homeworld)
But yeah it does seem to happen that the influence of HATEOS has often made api designers feel more ok with the somewhat lazy ”you can just make more requests” approach rather than making case by case decions to include some referenced data where it makes sense
Hi, how can i share my project to you for a little code review? I'm so appreciate to you if you will do it. That's will be informative for everyone who wants get better in the react, and state management
Join my discord and send me a GitHub link
VSCode-Theme name?
Material community high contrast
great video, i dont think there is a need to see your face through the whole video tho. maybe just the intro, the code is a little more important. Might be just me tho, there will always be the weird ones that wanna stare at you instead of the code.
It makes it more personal. Most people like the webcam last time I did a poll
I don't think useEffect() should be used here. Dan Abramov wrote alternatives for this in "You Might Not Need an Effect" that's found in the beta react docs.
I’ll need to reread, but how else do you fetch data on mount?
@@WebDevCody Good point, I didn’t think about that. In a real life project I would have solved it using react-query or SWR :) But maybe that was beyond the scope of this refactoring
@@johandejager3692 but I see what you mean, instead I could have fetched the new page data in the onClick handlers instead of watching the page variable. Now I don’t think watching a state variable to refetch something is necessarily bad, but maybe I’m the minority on that topic
Actually in “You might not need an effect” there is a section called “Fetching data” which describes a similar situation as the one in your video. There’s something interesting in that section about race conditions in there too that I _think_ applies to your example as well
@@WebDevCody I agree it’s fine in this situation. I’m just not a fan of useEffect because I’ve found it hard to debug in more complicated apps. When your component has like 5 effects that each update state, and that state is used in dependency arrays of the other effects, it becomes a nightmare 😂 I guess I’m just a little traumatized by that project 😜
How do you delete lines so quickly? 2:15
Cmd x
@@WebDevCody Thanks!
This isn't really refactoring of "react code". This is refactoring JS code which happens to be written in react.
Super misleading title when half the video is about requesting data and combining it in an object.
So you’re a gate keeper of the word refactor?
@@WebDevCody In other words: the chosen project has almost no react-specific code.
The refactoring is fine.. but it addresses problems which you have with pretty much any framework.
@@falias4 I see what you mean, but I do recall refactoring away two useEffects into one useEffect and removing some computed state we was storing. So yes, your gripe is the beginners code was basic, but it’s hard to refactor much more when the project is so small. A subscriber literally sent me a create react app project asking for help debugging and cleaning it up. If you have a better way you would have refactored I invite you to my discord, we’ve been discussing this code sample in more detail in chat.
Just a thought about classes/types: whenever I see api-data mapped and extended on the fly like this I do appreciate untyped languages and feel like they speed things up and result in less boilerplate. Just not having to deal with setting up custom types along the way (Imagine the multitude of various Person-ish types like Person, PersonWithHomeworld, PersonWithHomeworldAndSpecies, etc) saves some energy.
On the other hand, had there been types some of the initial confusion regarding the datatype of various homeworld properties could've been avoided.
*moves agnostic business Logic inside a useEffect*
Thats when i click off the video. Lot of great points in the video other than that.
Good job babe
Not bad. But most of this wasn't really refactoring. It was debugging. You didn't know what the app did and you were trying to get it to do a thing. Many of your changes were either stylistic in nature or were added without a strong feedback cycle to see if you were correct.
Completely understandable because I've done this in the past as well. It would have been great to either start with a unit test, a readability concern, or with a metric as the basis of the refactor.
Never make refactoring its own "thing" but instead it should mainly be around active dev to keep the app active by adding features and applying the boy scout rule.
Overall though nice work! Just be careful when you are debugging around without intent. This could lead a bunch of juniors down a bad path; creating more effort because "it should look/behave this way" instead of focusing on the application state with small changes over time.
Yeah this was debugging and refactor combo.
Since there was a bug from the start that prevented anything from rendering I think this approach was fine in this case.
It's a nice thing that you are trying to refactor something, but have you ever thought of optimization, which is regularly a crucial part of refactoring? You are fetching the same homeworld many times.
Yeah, optimization is import an as well. I didn’t really care in this example because the best way to optimize this would be to setup my own proxy server which caches all the data as requests come in and I’d have my ui hit that proxy instead of their api. Maybe I can do a video on that as well
@@WebDevCody Ohh, this sounds super interesting.
A Master Class in refactoring React code! - Thanks.
{2022-07-31}
Bro make a vid about vs code shortcuts
I have one already
Wow, that API is bad...
Yeah it’s pretty rough they don’t include fetching nested data. Maybe they do I didn’t read through the api
I prefer Vue to be honest...
Yeah sometime vue is more straight forward