This is way too complicated! - Code Review
Vložit
- čas přidán 6. 11. 2023
- Join The Discord! → discord.cosdensolutions.io
Repo → github.com/JesseOgunlaja/Task...
VSCode Theme | Font → Material Theme Darker | Menlo, Monaco "monospace"
Welcome to Code Review!
This is a series of videos where I review code that you send me or that I find online. I review the code as I would when I work with my clients. You will see how a senior developer looks and thinks about code in a variety of scenarios, learn about best practices and how to do things the right way, and learn how to become a better developer overall.
Enjoy!
Darius
The first adjustment you did in the custom hook: You can also do "as const" instead of writing out the types and it will work
aha yes you're right. Thanks a lot I brushed over that too quickly
That will make return read-only array
@@cosdensolutions Yeah, the whole issue is that when you're only returning a regular array, the order isn't set, so typescript just types the array as something like (User | Dispatch)[]. But if you wanna make sure that the types are inferred correctly by the order of the elements, you have to turn it into a tuple - either by using "as const" like mentioned or actually writing out the tuple.
@@sh8yt Shouldn't be a problem since you never mutate them directly anyways
I was about to comment the same thing haha
Great video, loving these reviews. Keep them coming :)
will do!
Just a quick tip, when you need to change the name of a variable, click 2 times over the variable and press F2, it will change the name in all ocurrences.
Nice thanks!
You can save one click by have the cursor just selected anywhere in the variable (no need to select the whole word) and press F2
@@voigtanyes my go to option
@@voigtanfor mac users?
Please share more videos on this series.
Love to know the best practices react code.
There should be no reason that a type assertion is needed for the return array of that custom hook. There are two reasons for that issue: the first one being that they didn't give useState a default value, and/or they did not type the useState correctly in the type arguments. The second which is not necessary, really, but the whole function itself is not typed with a return type, which could have solved the issue as well. This is just a simple matter of not thinking about the typescript prior to writing the code, and then having to go back and put a Band-Aid on something that wasn't well thought through.
A type assertion is perfect for the custom hook by just doing "as const". It's DRY and is quite clean
I am a junior dev that starts this journey of we dev for almost 3 months ago, and the way he just explains and code it along really make such an understanding of the concept of SRP even though it's in TS which i have none of the knowledge yet, but i can still understand what he refactor in this video. Good Job cosden, we need more senior dev like you to teach junior dev out there that is building their knowledge to become the best version of developer they are!
What do you mean "one thing"? "Thing" is a term that is so versatile it could refer to something as basic as "set the value of this variable" or as complicated as "render the whole page" and they would both be "one thing" from a linguistic standpoint.
If you think at the lowest possible level, the function still does several things. At what point does splitting functionality become not recommended?
There's no one size fits all answer here. But generally for single responsibility principle it should be one thing "logically".
So in the end it boils down to what makes sense in a given context. You can have one component that handles logic + ui for user signup or you can break it down into two components which handle logic and ui respectively. Both of these would be fine with SRP.
Contrary to above example, if you had a single component that does user signup and account management then I would argue its deviating away from SRP, even though they're somewhat logically related (you gotta sign up to manage your account etc.)
If you want rename a function, i suggest to use f2 it will update all function reference automatically. :)
This assumes he is using normal VsCode bindings hahaha, he’s using vim or VsCode-Neovim and best believe he probably got it rebound 😂😂 I know I do
I prefer 2 separate toast functions instead of 1 merged. Easier intellisense/autocomplete and fewer arguments to type each time.
But if I WERE to merge them, I would put the type before the text, because it feels awkward to read the end of the function to get the context that you need at the start. Whether something is an error or a success message is something I would like to know before I read the actual message itself.
Yeah that's a fair point!
I’d still like to decrease the duplication by creating showToast, but we have two options: either expose the showToast with type param, or make showToast private and still expose both successToast and errorToast. Both options are good.
@@daydreamer9469 Yes, I would also likely have some shared data between the 2 toast functions, probably in the way of a "default options" object that isn't exported. If the logic gets complicated but I still want to share it, a common underlying function makes sense. Whether or not to export that is optional though.
I would have the single function in the implementation, but create two wrapper functions that are equivalent to the original functions, just making use of that combined implementation. That way you reduce repetitive code without needing to adjust any style.
The code was perfect to begin with. As a rule of thumb: Avoid duplicating logic (such as business logic), but hell yeah duplicate options! The representation and behaviors of these two toasts can drift quickly apart, so there is no real benefit trying to keep "common" options.
o yeah ! Thanks for teaching us best practices! and more.. you deserve a good cold beer :D (or more :P )
Thanks Cosden for the recommendations I’ll merge with the PR.
Great video!
I disagree on passing Partial as parameter to the ShowToast. Mainly because you couple the rest of the codebase to the type of library you use for toasts.
In case you remove this library and decide on a different approach you essentially need to touch everywhere to remove it.
Move toastOptions outside the function. No need to create a new object to hold defaults on each toast call.
Loved the video ❤
"You should explore public repositories to learn and improve."
The repositories:
😂😂😂😂
awesome review
Getuser should have been as const , as from the function a tuple is returned
Amazing!!!!🔥🔥🔥
nice content ✅
Awesome 👍👍👍🎉🎉🎉
Interested to know how your ide knew to capitalize the setValue to setUser?
That is from an extension that deals with that - Multiple Cursor Case Preserve
exactly, made a video on that too not too long ago
Can you please share the name of the extension that fixes function names on the top?
Sticky headers, it's built into vscode
@cosden
One Question
In a parent component how many states can be defined i have a situation where i have to define like more than 10 states and through props i send the states and values to the children component is this is the right way to do it?
Hoping to get the response asap.
every state that you use you should ask yourself "who needs this?" and only put it on the lowest level possible where it is needed. You can have states to pass around through props, but if you find yourself having too many, then I would try to think about how you could re-structure it! Not every state needs to be in the parent
@@cosdensolutions
Can you provide me or made a shorts on this then it can be very helpful as a frontend developer i think that defining lots of states is too complicated and increases the line of codes also.
This sounds like a case where I would reach out for Context or state management lib.
Ok can you help with that 🙏
بسیار عالی بود. ممنون از شما
what is the name of theme?
Make video like this more and more
Were custom hook rules ignored intentionally?
No, I had similar functionality across my components of fetching the user and I decided to put it all in one hook. However I was still in the middle of whether it was a function or a hook. I had never used custom hooks before and decided to just try it. However I definitely need more practice with it 😂
Bro is doing things with typescript that even jesus itself cant forgive
Your "Code review" of toast was kinda bad.. All you did was refactor all of it's functionality that "fits your style". You didn't even talk about this line:
text != undefined && text != null && text !== "" which is, in my opinion, more troublesome, and which you lost in your refactor!
here - if you keep using == (or !=) then text != null would cover (text != undefined && text != null) in a single expression.
If you would want to know - whether this is undefined or null - then here must be used triple eq !== undefined or !== null.
But since here is typescript, and I would tust that there wouldn't be passed like a boolean, I would just go with if (text) which would cover all expressions
yeah, if you type cast your custom hook you are probably doing something wrong
Yeah you're right I went over it without mentioning it. That if check is actually not needed, text wasn't an optional so there was no need for an if check on it. And also, I didn't felt the need to check for an empty string as that responsibility should land on the dev to give it the string to render
Do react native one
lol dat getUser abstraction doe
why you need to spread the toast option, why dont pass the object as it is?
because if we spread it then we can override the default options with our custom options
I think it would work with just passing an object. React developers tend to spread like their life depend on it
How would you pass both objects as one without spreading?
Because you want the ability to pass the same option, but makes it modifiable.
Because if you spread two objects inside an object every field or method in the first object that is the same name. Will be overrided by the second object.
Like
objX = {...objY,...objZ} //every fields or methods that are the same in objY and objZ will be override.
Something like that.
I thought the same thing you did, before he tried to accept options override.
7:27 this condition if(text !== undefined && text !== null && text !== "") could be simplified as if( !text?.trim?.() ) return;
Why so many form api calls, seems inefficient. Could all be in one form
Could be, but maybe you want to allow individually to save them so they did it this way
Also you can change this condition
if (data.user != undefined && Object.keys(data.user).length !== 0)
to
if (data.user)
much cleaner and less weird (I've never seen anyone checking the length of the keys in an object to see if it's empty)
Empty objects still pass a truthy check.
if ({}) // true
Which means:
const data = { user: {} };
if (data.user) // = {} which is true
if (Object.keys(data.user).length) // = 0 which is false
@@MrMudbill Honestly if there is a possibility of an empty object for a user, this is more of an issue from the API. I either want a complete user object or nothing at all.
@@daydreamer9469 I mostly agree. I don't think you should _have_ to check key lengths of objects. But sometimes it's necessary because you're not always in control of what you receive.
@@MrMudbill of course, probably can’t help it if the case came from third-party APIs or third-party libraries
your videos are gem... Will you collab with me???
While there is nothing wrong imo about your toastFn ternary in showToast, I’d just shove the type directly by writing toast[type]. Two options from this:
1. toast[type](params)
2.
const toastFn = toast[type]
toastFn(params)
I saw my senior and he don’t know redux etc. He wrote only state 20 lines… In my opinion my senior is not senior.
Stagnated senior, not an expert.
2 minute in and i can already tell that the creator of this project is coding for 3-6 months
Big? boy i got 3k + lines of code on components xD
I was thinking the same, 641 lines is nothing compared to a project I've just taken over from some contractors the firm hired... It's now my job to make it work better, be quicker to scrap the whole lot and rewrite it.... 641. HAHAHA
that's too big! In most of my apps the general rule of thumb is a component should be < 200 lines of code. Because if you follow that rule, then you have to split them up and splitting them up is always a good idea imo no matter the project
Some of components at the end are going to be complex no matter what. They are supposed to be complex!!! and you just need to be patient and read it through!!!
Make file smaller wouldn't change the fact. In contrast, constantly doing the fake "abstraction" will quickly bloat your codebase with tons of small single usage file/component/hook. Every component will soon become unreadable without several file jumps. The refactor will only increase mental overhead and bug prone.
One of huge red flag every time I see people do constant refactor. Many times big file is fine, one or two code duplication is also fine. At the end, there is no such a golden rule.
if the 3k line component does one thing and does it very well, it's ok in my opinion. Perfectly encapsulated! Leave it alone!
@@doc8527 I get it, but I actually disagree! I felt like this in the beginning but as I've built more and more apps, splitting things came naturally and ended up making it easier to work with. Especially in teams on bigger projects. I know this seems tedious and I'm not going to force you to do it. But that's how React was designed from day 1 and it's unfortunate that developers go against that because they think they know best. It may very well work for you and small projects, but this isn't sustainable at the enterprise level where working well within a team and the success of the product matters
i have a single component that has 2500+ lines of code.
I'm sorry to hear that
just ropemaxx already
pro tip: Dont talk to your audience, talk to your "dream customer" or "dream avatar" or your viewer. Talk to your past self.
which dog made the typescript xxx him
This remind me why i hate React so much