This is way too complicated!

This is way too complicated! - Code Review

Рет қаралды 20,941

7 ай бұрын

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

Пікірлер: 99

@KumaDAce 7 ай бұрын

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

@cosdensolutions 7 ай бұрын

aha yes you're right. Thanks a lot I brushed over that too quickly

@sh8yt 7 ай бұрын

That will make return read-only array

@yitzchaksviridyuk932 7 ай бұрын

@@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.

@wlockuz4467 5 ай бұрын

@@sh8yt Shouldn't be a problem since you never mutate them directly anyways

@frstylol 2 күн бұрын

I was about to comment the same thing haha

@harag9 7 ай бұрын

Great video, loving these reviews. Keep them coming :)

@cosdensolutions 7 ай бұрын

will do!

@keyurpatel2472 4 ай бұрын

Please share more videos on this series. Love to know the best practices react code.

@Legend-bv3hi 7 ай бұрын

Thanks Cosden for the recommendations I’ll merge with the PR.

@softultraviolence5673 7 ай бұрын

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.

@cosdensolutions 7 ай бұрын

Nice thanks!

@voigtan 7 ай бұрын

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

@shankar99977 7 ай бұрын

@@voigtanyes my go to option

@user-zw1pz2pj3p 7 ай бұрын

@@voigtanfor mac users?

@nokroman 3 ай бұрын

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!

@Regeneration1996 7 ай бұрын

o yeah ! Thanks for teaching us best practices! and more.. you deserve a good cold beer :D (or more :P )

@ptolemyhenson6838 6 ай бұрын

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?

@wlockuz4467 5 ай бұрын

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.)

@hassanad94 7 ай бұрын

If you want rename a function, i suggest to use f2 it will update all function reference automatically. :)

@Hellbending 6 ай бұрын

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

@prakashraj4519 7 ай бұрын

Loved the video ❤

@PaulSebastianM 7 ай бұрын

Move toastOptions outside the function. No need to create a new object to hold defaults on each toast call.

@alexjohnson-bassworship3150 7 ай бұрын

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.

@zackarysantana8208 7 ай бұрын

A type assertion is perfect for the custom hook by just doing "as const". It's DRY and is quite clean

@MrMudbill 7 ай бұрын

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.

@cosdensolutions 7 ай бұрын

Yeah that's a fair point!

@daydreamer9469 7 ай бұрын

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.

@MrMudbill 7 ай бұрын

@@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.

@ptolemyhenson6838 6 ай бұрын

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.

@IIGoofyII 16 күн бұрын

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.

@dthrt 7 ай бұрын

Amazing!!!!🔥🔥🔥

@trtrion1999 7 ай бұрын

awesome review

@parvesh8227 7 ай бұрын

Getuser should have been as const , as from the function a tuple is returned

@peasantlord135 7 ай бұрын

"You should explore public repositories to learn and improve." The repositories:

@giyoka2609 7 ай бұрын

😂😂😂😂

@gavinlindridge 7 ай бұрын

Interested to know how your ide knew to capitalize the setValue to setUser?

@harag9 7 ай бұрын

That is from an extension that deals with that - Multiple Cursor Case Preserve

@cosdensolutions 7 ай бұрын

exactly, made a video on that too not too long ago

@appwala3728 7 ай бұрын

Make video like this more and more

@oguznsari Ай бұрын

nice content ✅

@tareksaid2427 7 ай бұрын

Can you please share the name of the extension that fixes function names on the top?

@cosdensolutions 7 ай бұрын

Sticky headers, it's built into vscode

@user-cm2nz9ik4v 7 ай бұрын

@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.

@cosdensolutions 7 ай бұрын

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

@user-cm2nz9ik4v 7 ай бұрын

@@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.

@wlockuz4467 5 ай бұрын

This sounds like a case where I would reach out for Context or state management lib.

@harshvikramsingh5266 5 ай бұрын

Ok can you help with that 🙏

@atuloraon9610 7 ай бұрын

Awesome 👍👍👍🎉🎉🎉

@user-op3mj5kq5c 7 ай бұрын

what is the name of theme?

@serhgoch 7 ай бұрын

Were custom hook rules ignored intentionally?

@Legend-bv3hi 7 ай бұрын

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 😂

@RA-xx4mz 7 ай бұрын

lol dat getUser abstraction doe

@samerallahham2182 7 ай бұрын

Do react native one

7 ай бұрын

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

@LuKaSSthEBosS 7 ай бұрын

yeah, if you type cast your custom hook you are probably doing something wrong

@cosdensolutions 7 ай бұрын

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

@wecreds5859 5 ай бұрын

Bro is doing things with typescript that even jesus itself cant forgive

@IlSuperHeron 7 ай бұрын

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)

@MrMudbill 7 ай бұрын

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

@daydreamer9469 7 ай бұрын

@@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.

@MrMudbill 7 ай бұрын

@@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.

@daydreamer9469 7 ай бұрын

@@MrMudbill of course, probably can’t help it if the case came from third-party APIs or third-party libraries

@FourSightfulGaming 2 ай бұрын

pro tip: Dont talk to your audience, talk to your "dream customer" or "dream avatar" or your viewer. Talk to your past self.

@apoorvgupta9680 7 ай бұрын

why you need to spread the toast option, why dont pass the object as it is?

@rajkun69 7 ай бұрын

because if we spread it then we can override the default options with our custom options

@LuKaSSthEBosS 7 ай бұрын

I think it would work with just passing an object. React developers tend to spread like their life depend on it

@cosdensolutions 7 ай бұрын

How would you pass both objects as one without spreading?

@asagiai4965 7 ай бұрын

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.

@daydreamer9469 7 ай бұрын

I thought the same thing you did, before he tried to accept options override.

@Tyheir 7 ай бұрын

Why so many form api calls, seems inefficient. Could all be in one form

@cosdensolutions 7 ай бұрын

Could be, but maybe you want to allow individually to save them so they did it this way

@weroro 5 ай бұрын

7:27 this condition if(text !== undefined && text !== null && text !== "") could be simplified as if( !text?.trim?.() ) return;

@daydreamer9469 7 ай бұрын

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)

@ManoharBatra 6 ай бұрын

your videos are gem... Will you collab with me???

@mehmet19059 7 ай бұрын

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.

@daydreamer9469 7 ай бұрын

Stagnated senior, not an expert.

@MegaTragos 7 ай бұрын

Big? boy i got 3k + lines of code on components xD

@harag9 7 ай бұрын

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

@cosdensolutions 7 ай бұрын

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

@doc8527 7 ай бұрын

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.

@doc8527 7 ай бұрын

if the 3k line component does one thing and does it very well, it's ok in my opinion. Perfectly encapsulated! Leave it alone!

@cosdensolutions 7 ай бұрын

@@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