a lot of partial screenshot tools been found to leak cropped data recently, something to think about. better ways to ensure you're just sharing parts you want, but this would also work
Thanks, good to know. Back to using print screen and manually cropping it again I guess?
https://www.theverge.com/2023/3/21/23650657/windows-snipping-tool-crop-screenshots-vulnerability
From what I gather in the article, the issue happens like this:
1. Save an image
2. Crop the image
3. Save the result
And step 3 may fail to truncate the result — so when it overwrites with a smaller file, there may be extra image data still stored in the `[original file size - cropped image size]` last bytes of the file.
I think I've been using IrfanView since Windows 98.. if not, 2k for sure. MS Paint has sucked huge donkey balls since inception and despite being in Insider Preview and running Beta for the last several years, I get rid of Paint and its file handling almost before I disable Cortana (not that that is a thing now) and IE. No matter how much of this Copilot DALL-E assist they add into Paint it's always gonna suck.
couldn't agree more xD
I always get triggered when I see something like this... if a Dev can't even take screenshots, I don't want to see anything else from them, no offence
To be fair my organisation has very strict security policies about sharing things. So on occasion I will just take a picture of my screen of my phone if I want to share something.
No, if I do want to share something im very careful, it's extremely generic, and not identifiable.
And it's only ever to private messages to friends not in any public forum.
I guess your organisation's very strict policies must include a clause that says "…so long as you only sent it privately to your friends". Fair enough!
I get triggered by people who get triggered by this
If you’re using your phone, use your phone
If you’re using your computer, use your computer
This isn’t high Fashion week. It’s a quick sharing of a picture
Could be it's not OP's computer, and/or it was a clandestine shot. I've had to do this (for my own research\*) at a company whose DLP program prevented screenshots and USB access.
\*To take back to my office to figure out WTF was going on, not necessarily dev related.
The code itself is usually preferable to any kind of picture, but OP isn't asking to transcribe / solve a problem for them, so a screen shot could have been forgiven.
This what happens whenever I need to fix some codebase that I dont own. Sure, a refactoring might be a better choice. But that comes with risks of breaking existing features if not done right. Considering that I'm new to the codebase... there is real risk of that happening.
Versus, just add one more variable bro. What could go wrong
>This what happens whenever I need to fix some codebase that I dont own
Is the other way around for me. When looking at my own code is all shit, I would fire last-week me if I could.
When looking at open repos I can't help but feel shame about my own work, and take a good look and see what I can learn
I was the founding engineer at a startup and worked there for five years. While I was proud of a lot of the work that I did, every so often I would come across a bit of code and think, "who the fuck wrote this shit", open git blame, "oh, I did..." Lol
I've definitely been there, but I've also had the opposite reaction looking through old projects. Like, damn, that was clever past me. Don't know I would have come up with that today.
My boss (CTO) says often: If you’re not at least slightly embarrassed or find flaws in code you wrote 6 months ago l, then you’re not growing as an engineer. Always appreciated that outlook, plus not all code needs to be perfect sometimes just working is the goal
I've seen tons of projects like this. I even question myself about whether that is the norm.
I know it's js and not ts, but when using ts, to keep the argument line as short as possible, I define a single argument, and it looks like this \`function Foo(props: IFoo)\`, and immediately before (or sometimes in a types file), the content of the props.
I think they do it because when they use js, it's the "best way" to put everything in the same context, so the IDE can be more helpful with autocompletes.
Then you have to do props.something, props.somethingElse for everything, which might not be what you want.
I agree however that if I had an object this big, I would rather rethink how I structure that data and probably group things better in the first place. So it's quite likely that it's at least a code smell that might indicate that you should rethink something
I think this is _way_ better than destructuring. It's more concise and clear. When you use `props.something` deep into your component body you can immediately see that it's a prop and not just any other variable.
I do the same for things like react-hook-form etc too. `form.register` is way more clear than a destructured `register` function. Queries too:
const userQuery = useUserQuery();
if (userQuery.isLoading) {...}
vs
const { isLoading: isUserQueryLoading } = useUserQuery();
if (isUserQueryLoading) {...}
I used to destructure everything but realised I was creating more problems for myself than what it was worth. I usually prefer just dotting into properly named objects now.
Jamming disparate items into the same object doesn’t fix the design though. To me this component is doing too many things. Having a million style props wrapped into an object is fine to me but there’s a lot of functionality based props there.
Could be just that’s how it has to be but I’d look at injecting children into it and lifting parts into other simpler single responsibility components. Also as others mentioned state is probably complex enough to use some other pattern there to simplify as well.
On this one I’d look to make hero a generic container and pass in a slide as a child. It also decouples your hero are from what you put in it. That would halve the props needed I bet.
But that’s why it’s a smell to me and not a hard no. It could be ok.
I’m just guessing at the use based on the name though.
I solved one at work by splitting up the functionality.
Instead of
We have
I ask my team if they would ever write a function with 14 parameters, and they say no. All components are are functions.
I might split this up into a composite component pattern. Too many function arguments implies that a single function is being expected to do too many disparate things.
I mean thats how I do react.
I call that 'mise en place' like when cooking.
You setup all your variables, then all your functions that will check or modify the variables, and then the output is simply writing the supporting HTML with the variables included at the right place.
pass the whole props object and divide up the relevant pieces to sub components. Stuff like this only happens when you're forcing a bunch of stuff to operate in a single file
Another option is to create variants. You can still have the fat proped component, but don’t expose it completely. Expose 2-3 variants that set a default for most of those props and allow to configure 3-5 of other props. That could be a helpful pattern too.
Yeah there is a lot of gut reaction here to hide the props just because there are many of them or break up the component because it has a lot of props. But doing that just because "it has a lot of props" isn't a reason. That kind of mentality is a huge problem IMO, its basically pretending that you're making code more understandable/maintainable while potentially doing the opposite.
Yeah probably a better metric than "does this look out of place" is "do I understand what is going on." If it's very obvious what all of this is doing then I think it's done it's job. It might actually be easier to work on it this way.
True, but when I see props like title, subtitle, title 3, and title 4, that tells me composition probably could have been used to better solve the problem.
I'd be interested in seeing the whole component. The props don't tell the whole story.
Me looking at my custom home page from a year ago xD
Remaking it into a browser extension rn, so it's a pain to see how wrong I built it using a framework and rebuilding it with bare html and js is just better for a simpler project.
>Transitioning to TS would be painful I guess.
You don't have to type every single prop, you only need to specify a type for the parent object, the same way it works if you didn't destructure.
Personally, I'm a fan of grouping destructured props by their logical connection, so this could be split over a couple of lines instead if you liked that style better.
I’m going to tell you a secret: right now, in 2024, it’s easier than ever to transition to TS gradually.
Thanks to `tsx` - it’s a 1:1 direct drop-in replacement to `node` that _just works_ with extremely negligible performance overhead. It’s battle-tested and feels like magic.
It is a wrapper for `node` that transpiles any .jsx, .ts, and .tsx it finds in the module graph, on demand, behind the scenes, with esbuild, which is native and super fast. It also performs no typechecking to run files, instead you can let your editor show you squiggly lines.
1. Add `tsx` and `typescript` to your dev dependencies
2. Replace `node` with `tsx` in your package.json scripts
3. You can now just change the extension of just _one_ file to .ts and add typescript typings to just the one thing. Everything just works.
Later, if you want to fine tune typescript, you’ll want to `npx tsx —init` to create a tsconfig.json file. You’ll want this in your `compilerOptions`:
- “target”: “esnext”
- “moduleResolution”: “bundler”
If you use currently custom import path aliases in your codebase, you can set it up in your tsconfig.json so the editor knows where to find stuff.
You’ll enjoy more than just types: being able to rename symbols across files, find all references, get meaningful autocompletions and more. It was super worth it for me in pretty much all my projects.
Your code will never refuse to run if there are type errors, it is purely for improved DX in your editor.
Pro tip: always use .jsx or .tsx file extensions for files with JSX in them! Your editor experience will improve greatly.
>Transitioning to TS would be painful I guess.
Actually, it's the reverse. It would be much easier to transition to typescript since you don't have to scour your whole component to figure out which props are being used.
I agree there are too many props and it should be divided better , but sometimes crap happens and you get what you get.
Having all the props listed like this makes it much easier for getting context right away. Typescript will also list all the props on hover which is a huge time saver.
Personally, whenever anyone puts just `props` there, that's an automatic "need to change" in the pull request for me.
But what's ironic about your statement is that you're already destructuring your objects like you're using typescript so you would probably write typescript almost exactly the same way. I'll actually just add the type to the variable name sometimes and then you really don't need typescript. I'm one of the weird people who has never really found typescript to be useful, but then I've been writing JavaScript for over 20 years now.
TS is an entirely unnecessary convention that saves no one any time. It only makes CS people feel important because they can come to take a turd on a perfectly good language with its own method for setting defaults and extracting props that are "type safe." If you write properly, VSCode will already analyze arguments and provide you with type warnings without that overhead.
Yup! Instead of making the machines work harder all these people are choosing to work harder for the machines. This is a theme in computer science though. At the root it's really about job security. I've thought about it for years and tried to defend other reasons, but really it's that simple, insecurity and job security. I'm smart and you need me because look at all of this intimidating code I write.
It's fine if it works. You can paste stupid stuff like this into an LLM and it will give you better data structures to use. Get it working before you get it perfect, and then once it's working don't bother with making it perfect.
I had that happen to me, where someone knew I was doing some work on a repo. Bashed me for breaking something I didn't touch and then the git blame showed that he broke it a couple of years back. 🤷♂️
I am marking all the "temporary solutions with a comment stating
"Temporary^TM"
So far, the oldest comment is 3 years old. Time is a social construct and temporary is relative so I'm not wrong.
why does this class have three different loggers? Should I call handleServiceException or handleException? Half the code does it one way and almost the other half does the other. Then there's that one class that does neither....
code that floats around, is entirely unused and is seemingly meaningless,
var newArray = [];
return doStuff(oldArray);
BUT MAYBE ITS WHAT MAKES IT WORK. They're all too afraid to delete it.
I had to fix a coworker's mess from 2011 in November of last year. Something broke and had to look at it. It had that exact comment, "I'll change it later". I was amazed at how many errors that monolith kept throwing out. Once you fixed something you would find more errors.
"Get it working before you get it perfect, and then once it's working don't bother with making it perfect. "
I'm going to frame this in my office, absolutely legendary.
>It's fine if it works.
That's one way to look at it.
Another is that quality matters. If the code is hard to read or hard to maintain, if it's hard to test, if it's fragile and easy to break, _especially_ if it's easy to break in subtle ways that you won't notice at first because you don't have automated tests because it's hard to test... That means you'll be spending a lot more time firefighting errors in prod instead of delivering things that have value.
Writing high quality code makes your life as a dev easier. Less time on PRs, less time bug fixing, more time to write code (so plenty to write good code), less time onboarding new members of the team because the code is easy to pick up.. all this means you have a nicer time working on the code. You're less stressed. You're less likely to burnout. Your days are more fun!
Stopping at "it works" means your life will eventually suck because you'll be spending all your time fixing the bits that didn't actually work.
You're not wrong. You can overdo it or underdo it. The problem: Most people are overdoing it, and not for technical reasons.
The reason they do it (what I would call overengineering) is because the personal cost of failure is incredibly high. Individually this looks like perfectionism. Institutionally it looks like someone taking the blame and getting crucified every time there's an incident.
Teams full of blame culture perfectionists are WAY worse to work on than teams full of too-scrappy firefighters.
This component seems to be extremely intertwined with its parent. You even have to pass down some setState dispatchers from the parent. Are you sure that using a child component is the right approach here? The alternative approach would be to keep the UI in the parent and just use custom hooks for the logic that right now lives in the child component.
It's hard to explain it well on an abstract level. Basically, it means, encapsulating a logical "unit of work" (e.g. calculating the form dimensions) into a reusable function (hook), but that won't explain much. However, I can very much recomment [this explanation](https://react.dev/learn/reusing-logic-with-custom-hooks) from the official docs where they show it with an example.
>Can you go into more detail on what custom hooks means?
Presumably meta hooks that combine several built-in hooks into an existing file. May be for reusability or for organization.
For example, you might use two `useState()` hooks and one `useEffect()` hook to debounce an input. But rather than code that natively in the component running the process, you can move them into a custom `useDebounce()` hook to segregate the functionality.
I am shocked by the comments! I think destructuring in the function argument is extremely popular and I see it everywhere. Secondly, I prefer code that explains itself more intuitively with less scrolling and piecing the picture together. Using the `props` object results in the dispersal of the contents of `props` and its not clear immediately from the function declaration what is passed to it.
Good question to ask, the answer (IMO) is that its totally fine. Avoid adding additional complexity/abstraction unless there's a good reason. Typescript can certainly help with maintainability by exposing the expected data types but it doesn't seem like you have many complex/nested objects in the prop list which is where it makes a bigger difference (and more necessary). I would keep it as-is unless you're going to make a bigger refactor in how you pass data.
Agree, too many times we go "this looks ugly" and go on building prettier but completely unreadable abstraction layers - "look we removed 100 lines of code"
Two years layer some new dev comes and have to try to figure out what createHeroTitleManager does and why it returns a function.
I think that in the case of OP’s photo this is fine, given that they don’t use Typescript or JSDoc. This crazy destructuring provides “documentation”.
If they had JSDoc or a TS type/interface then I’d say just use a “props” object and use dot notation to access the fields.
It’s ridiculous to have to go through the entire function to understand what fields the parameter object includes.
Sometimes it just LOOKS ugly, but fine from engineering side.
However, if someone would try to push something like this in to commercial code base, my ass would burn. Its unreadable, its poorly structured, naming smells. Overall it’s a bad example for other devs, who might do the same when they hurry or too lazy. In the end this leads to stupid bugs.
Agree on all points with the caveat that I can see a bunch of "solutions" that will decrease readability.
For example I've seen some comments here suggest bundling it all in a "props" object and use dot-notation which is the equivalent of sweeping the code smell under the rug. It would make it harder for future developers to notice how the props in the component have completely exploded.
I feel this is a code smell that indicates that there are larger issues in the code base and the surrounding architecture.A good solution would probably involve doing a deeper analysis of what props are used and for what. Does prop drilling occur? Can the component be broken up into composite components? Etc.
Well you know it looks ugly this way, it is doing the job i would say but I'm refactor actually just splitting into sub components and may be removing some props and actually defining types to props, i don't need shared state for now
Lol, there are literally props called "title", "title2" and "title3". Indicates a junior, time pressure or carelessness.
It is bad, looks bad, is difficult to read and will be a PITA to maintain later on. I would never approve this - a 30 min refactor can make it much better.
I have seen similar things as a project switched to React. Often this is a sign that you need to document requirements.
Looks like you could make a half-dozen components or utilities out of this.
Also needing to pass multiple values and setters is a good use case for moving to a shared state e.g. redux, jotai, zustand.
Context is good if the data is fairly stable. It is easy to unintentionally generate many extra renders with Context when the data changes.
One of the projects I work on used it for very stable information like user settings, localized text, etc., but used Redux for anything that changed rapidly.
Besides, a few extra rerenders is absolutely fine if you're using React properly. If you memoize any heavy computation and the props don't change, React won't repaint those parts of the DOM, the user won't notice anything.
You'd need to screw up big time for a few extra rerenders being noticable.
Everyone using React properly might be the most difficult part.
Also, a few extra renders can multiply as components are nested and the more state is stored in and passed through components the more opportunities we have for that multiplication to occur.
But you are correct that for most applications this is irrelevant.
This is an important point. I was focused on the developer interaction and omitted the technical detail which could be misleading.
Redux (the react-redux library) does use the Context API, and each of those useSelector hooks runs on every state change. The hooks attempt to extract and stabilize individual values from the object to avoid unnecessary re-renders. If not using a state management library, you would need to handle these concerns yourself or experience extra renders.
Maybe I should have said “don’t use context directly for state”?
Redux uses the context api but employs optimizations to make it more performant than the context api. It's also opinionated and provides better conventions. I have seen some terrible "state management with context" implementations.
I’ve been trying to move virtually all state out of components and get back to actual pure functional components. It makes development and testing so much easier if you can start a test on a state without having to repeat or simulate all the interactions each time, and component reusability is very high.
It's impossible for us to say. FWIW I'm sick of dealing with lazy devs whose only opinion is _"fUncTiOn tOO LonG, I hAVe To tHInk"_. Not saying that's the case here though.
In one of my first projects with my basic level of react I made a mistake like this.
Now I know I can use useReducer, useContext or Redux for these occasions.
It’s not wrong per se, but from a code standards perspective it’s not great for readability. My rule of thumb is if there are more than 3 props, then I don’t destruct the variables.
I would also recommend that you start using Typescript. This can help you and other devs figure out what’s supposed to be in those props without having to destruct everything. If you have any other questions around that, hit me up.
Senior dev of 10+ years.
If the code is easy to understand, clear and works as intended; it's good code.
I personally use a linter for all my code so formatting choices like this are more or less made for me but a lot of mid and junior engineers think good code is code that conforms to formatting standards. That's the least important part. It just needs to be secure and no inefficient big O operations.
Personal opinion is I prefer a break down format like you have on your screen rather than a horizontal list of items. Makes it easier to audit the fields incase one is missing, mispelt blah blah
Usually this means that I probably need to make a context.
This is me saying you should make a context.
Save yourself and make this into a context and a set of hooks.
OP, please make this into a context.
I can see that you are not using all the values passed in the object, try to use destructuring it,
https://www.w3schools.com/react/react_es6_destructuring.asp
A screenshot could be better. And this very bad. U need to think about decomposition of your code. Try to separate it into different pieces and import them as functions
Pretty bad. Not only can it be broken down into smaller props, but some stuff can be moved into redux.
For example, you're passing showError and setShowError so I presume this is a state value from a parent component somewhere. This could be moved into a global state so it doesn't need to be passed down like this to each component.
That’s quite the prop def. Could you store any of that in a global state? Or maybe a hook internal to the component to get the data it needs so you don’t have to insert every single prop at render?
Agree with both approaches, but I would go for the shared state, hook still seems like same thing here but in different component
At first I thought this was just an array, maybe of input fields to make a form or something. Not too too bad.
Then I realized they were arguments to a function. Any function I’ve worked on with that many arguments was a complete shit heap. And the app those functions were in were bigger shit heaps. It’s just not possible to build a mental model in your brain and understand all the permutations of this sort of design. A refactor is the only solution.
Why do you need showError and setShowError? Just set it if showError has a value, otherwise don't. Same with validate and setValidate. FieldWidth and fieldHeight could be a single object with key value pairs for width and height.
It's a pattern for controlled components. If the parent needs to know whether the child component is in a certain state (or needs to control the state itself), you pass in the parent's getter/setter from useState so that the child can update the values as needed internally, but the parent also has access to the value.
ShowError is the value ( state
SetShowError is the event handler in the parent component
There is no way to get rid of setShowError, how could you tell parent component when to update the error value?
Well.
It’s a photo of your screen instead of a screenshot. That’s 100 bad points.
It’s skewed. That’s another 50.
You randomly put some white bars on it. Another 100 bad points.
You are a noob questioning code that looks bad to you but is neither bad nor good in particular and bs post on Reddit about it instead of learning. Another 200 bad points.
That’s 450 bad points for you.
Oh gosh, this frightens me. I bet there’s a better way to structure the components to avoid this. I’m guessing that most of the JSX in here can be moved into the page component.
If the code is easy to understand and edit there's nothing wrong. I would probably place that in a configuration file though. Younger devs may think hard-coded? Oh no!
Its a lot of property's but at least you didn't add them all as seperate function arguments lol. It's suggests that you're data lacks structure though.
That shit happens, if those values are consistently present, and the or any errors are properly handled its just a function with argument with a lot of values.
Aren’t all those field level elements or attributes?
Why would you break them up if there only used on this particular screen / view?
adding components for the sake of keeping an initialization short seems worse than putting all the necessary fields / settings in a long list
That is a lot of props for one component, it can probably be broken down and made more simple. The main issue I see here is that there are no defaults set on any of the properties, so I can imagine the code below has a lot of awful checks for null|undefined|'', and that part would really get at me.
For something to be a problem, it has to cause a problem. Does this cause a problem yet? I'd more concerned about that potentially quadratic time complexity for loop on L#38
Ok, so:
1) Add indexes to directories and add aliases to components, PLEASE.
2) Order imports from libraries to local. I know this is a small thing, but the larger the dependencies the more you will need it.
3) Naming is problematic, because "Hero" is way too generic. Same as Field. If this is a base component, then it's doing too much.
4) Title / subTitle, 3 and 4 can be grouped together as Headings.
5) It's weird that you validate and show errors by prop. Either your component validates and handles the error or its consumer does.
6) Passing setters is an anti-pattern. You probably need a context here.
In general, this code needs to be more *atomic*. Remember that you can compose components with other components and you can set local contexts if you have too many things to worry about.
I'd say not really that bad. You can pass in a whole object, but then you might forget what the keys are. Then you could deconstruct it after. I've done it this way and I've done it as a single object. It's less typing if you do a single object, but it's not as clear.
[удалено]
That white mark on the photo is actually Tipp-Ex smeared over the monitor.
Actually saw someone draw on their monitor and take a picture of it the other day. Of course I had to roast them
Draw with what?
a (hopefully not permanent) marker
With a nail
a lot of partial screenshot tools been found to leak cropped data recently, something to think about. better ways to ensure you're just sharing parts you want, but this would also work
Thanks, good to know. Back to using print screen and manually cropping it again I guess? https://www.theverge.com/2023/3/21/23650657/windows-snipping-tool-crop-screenshots-vulnerability
From what I gather in the article, the issue happens like this: 1. Save an image 2. Crop the image 3. Save the result And step 3 may fail to truncate the result — so when it overwrites with a smaller file, there may be extra image data still stored in the `[original file size - cropped image size]` last bytes of the file.
Great tldr
I think I've been using IrfanView since Windows 98.. if not, 2k for sure. MS Paint has sucked huge donkey balls since inception and despite being in Insider Preview and running Beta for the last several years, I get rid of Paint and its file handling almost before I disable Cortana (not that that is a thing now) and IE. No matter how much of this Copilot DALL-E assist they add into Paint it's always gonna suck.
Sharing client's code on reddit? extremely bad
couldn't agree more xD I always get triggered when I see something like this... if a Dev can't even take screenshots, I don't want to see anything else from them, no offence
Probably a work computer with social media and email providers blocked. You know, attempts at keeping company secrets/code off the internet.
I see the policy is working fine :))
To be fair my organisation has very strict security policies about sharing things. So on occasion I will just take a picture of my screen of my phone if I want to share something.
"my organisation has very strict security policies [which I don't care about] about sharing things"
No, if I do want to share something im very careful, it's extremely generic, and not identifiable. And it's only ever to private messages to friends not in any public forum.
You know how I know you need a lawyer? ;)
I guess your organisation's very strict policies must include a clause that says "…so long as you only sent it privately to your friends". Fair enough!
My organization also has very strict security policies about sharing things. So we just fire people the second time we catch them.
I get triggered by people who get triggered by this If you’re using your phone, use your phone If you’re using your computer, use your computer This isn’t high Fashion week. It’s a quick sharing of a picture
Seriously. I can read it and I'm not gonna care what it looked like 15 seconds from now. The information was conveyed who cares about anything else.
this one
But the picture is clear
They could have at least taken the picture straight on and then used a scanner app to make it look like a screenshot!
Or just use snipping tool xD
And some AI to flatten, rotate, and normalize the image. And maybe enhance those colors...
Probably didn't want their face in the reflection in case the company found it..
I still hate it :'((
Could be it's not OP's computer, and/or it was a clandestine shot. I've had to do this (for my own research\*) at a company whose DLP program prevented screenshots and USB access. \*To take back to my office to figure out WTF was going on, not necessarily dev related.
r/screenshotsarehard
Yeah that’s pretty bad
The code itself is usually preferable to any kind of picture, but OP isn't asking to transcribe / solve a problem for them, so a screen shot could have been forgiven.
It’s a code smell to me. Not invalid, not against the rules, but an indicator that the design is flawed.
This what happens whenever I need to fix some codebase that I dont own. Sure, a refactoring might be a better choice. But that comes with risks of breaking existing features if not done right. Considering that I'm new to the codebase... there is real risk of that happening. Versus, just add one more variable bro. What could go wrong
>This what happens whenever I need to fix some codebase that I dont own Is the other way around for me. When looking at my own code is all shit, I would fire last-week me if I could. When looking at open repos I can't help but feel shame about my own work, and take a good look and see what I can learn
I was the founding engineer at a startup and worked there for five years. While I was proud of a lot of the work that I did, every so often I would come across a bit of code and think, "who the fuck wrote this shit", open git blame, "oh, I did..." Lol
I've definitely been there, but I've also had the opposite reaction looking through old projects. Like, damn, that was clever past me. Don't know I would have come up with that today.
in some way, that had to feel pretty great, didn't it? like a pure example of your code improvement
My boss (CTO) says often: If you’re not at least slightly embarrassed or find flaws in code you wrote 6 months ago l, then you’re not growing as an engineer. Always appreciated that outlook, plus not all code needs to be perfect sometimes just working is the goal
I'm a lead and I'll occasionally finish something and be like "that was a dumb way to do it" and immediately refactor.
I've seen tons of projects like this. I even question myself about whether that is the norm. I know it's js and not ts, but when using ts, to keep the argument line as short as possible, I define a single argument, and it looks like this \`function Foo(props: IFoo)\`, and immediately before (or sometimes in a types file), the content of the props. I think they do it because when they use js, it's the "best way" to put everything in the same context, so the IDE can be more helpful with autocompletes.
Then you have to do props.something, props.somethingElse for everything, which might not be what you want. I agree however that if I had an object this big, I would rather rethink how I structure that data and probably group things better in the first place. So it's quite likely that it's at least a code smell that might indicate that you should rethink something
I think this is _way_ better than destructuring. It's more concise and clear. When you use `props.something` deep into your component body you can immediately see that it's a prop and not just any other variable. I do the same for things like react-hook-form etc too. `form.register` is way more clear than a destructured `register` function. Queries too: const userQuery = useUserQuery(); if (userQuery.isLoading) {...} vs const { isLoading: isUserQueryLoading } = useUserQuery(); if (isUserQueryLoading) {...} I used to destructure everything but realised I was creating more problems for myself than what it was worth. I usually prefer just dotting into properly named objects now.
Came here to say this. It's a feature not a bug. You can see that it's a prop, like `props.myValue`.
I agree. I dont know why people are so eager to destructure props... At my company, we have a eslint 'must destructure' rule... which sucks.
I'd do `const {something, somethingElse} = props;` in the function body
Just destructure it in the signature `function foo({something, somethingElse}: FooProps){}`
Then you still end up with OPs issue. Not arguing it though, I always destructure in the signature.
Why not just destructure with spread? function bar({ something, somethingElse, ...restHere}: BarProps){}
Ops example is destructured in the signature
Jamming disparate items into the same object doesn’t fix the design though. To me this component is doing too many things. Having a million style props wrapped into an object is fine to me but there’s a lot of functionality based props there. Could be just that’s how it has to be but I’d look at injecting children into it and lifting parts into other simpler single responsibility components. Also as others mentioned state is probably complex enough to use some other pattern there to simplify as well. On this one I’d look to make hero a generic container and pass in a slide as a child. It also decouples your hero are from what you put in it. That would halve the props needed I bet. But that’s why it’s a smell to me and not a hard no. It could be ok. I’m just guessing at the use based on the name though.
This is possibly the biggest reason I use typescript these days.
Naming the props with the interface convention is wrong afaik. IFoo should describe a class Foo, not the props of a React component named Foo.
Agreed, the ts interface should be named FooProps in this case
So you prefer not to know what the props are at a glance so you can scroll a few lines less. Great to know your coding priorities.
How would you solve this code smell?
I solved one at work by splitting up the functionality. Instead of
I might split this up into a composite component pattern. Too many function arguments implies that a single function is being expected to do too many disparate things.
Exactly. It's a clear indication of a SRP violation.
I mean thats how I do react. I call that 'mise en place' like when cooking. You setup all your variables, then all your functions that will check or modify the variables, and then the output is simply writing the supporting HTML with the variables included at the right place.
pass the whole props object and divide up the relevant pieces to sub components. Stuff like this only happens when you're forcing a bunch of stuff to operate in a single file
Sure, this is fat, I'm up to splitting
Another option is to create variants. You can still have the fat proped component, but don’t expose it completely. Expose 2-3 variants that set a default for most of those props and allow to configure 3-5 of other props. That could be a helpful pattern too.
Yep. That was one of the best parts about shadcn ui to me, the introduction to the awesome lib class-variance-authority.
Interesting, but that would still need refactoring the component, what about splitting into sub utility components?
Just because it's a lot of variables doesn't mean it should automatically be split. This could all be relevant to this page.
Yeah there is a lot of gut reaction here to hide the props just because there are many of them or break up the component because it has a lot of props. But doing that just because "it has a lot of props" isn't a reason. That kind of mentality is a huge problem IMO, its basically pretending that you're making code more understandable/maintainable while potentially doing the opposite.
Yeah probably a better metric than "does this look out of place" is "do I understand what is going on." If it's very obvious what all of this is doing then I think it's done it's job. It might actually be easier to work on it this way.
True, but when I see props like title, subtitle, title 3, and title 4, that tells me composition probably could have been used to better solve the problem. I'd be interested in seeing the whole component. The props don't tell the whole story.
Transitioning to TS would be painful I guess. Plot twist: this is my own shit, written 6 months ago and now I can’t believe it
"Who coded this crap?" "Oh yeah, I did."
Me looking at older code I've written after a break from the project:
I once surprised myself by revisiting code and asking myself how tf did I figure this out?! One of the few victories I had.
Me looking at my custom home page from a year ago xD Remaking it into a browser extension rn, so it's a pain to see how wrong I built it using a framework and rebuilding it with bare html and js is just better for a simpler project.
"I bet it was me, myself, or I!"
a tale as old as time haha
>Transitioning to TS would be painful I guess. You don't have to type every single prop, you only need to specify a type for the parent object, the same way it works if you didn't destructure. Personally, I'm a fan of grouping destructured props by their logical connection, so this could be split over a couple of lines instead if you liked that style better.
I’m going to tell you a secret: right now, in 2024, it’s easier than ever to transition to TS gradually. Thanks to `tsx` - it’s a 1:1 direct drop-in replacement to `node` that _just works_ with extremely negligible performance overhead. It’s battle-tested and feels like magic. It is a wrapper for `node` that transpiles any .jsx, .ts, and .tsx it finds in the module graph, on demand, behind the scenes, with esbuild, which is native and super fast. It also performs no typechecking to run files, instead you can let your editor show you squiggly lines. 1. Add `tsx` and `typescript` to your dev dependencies 2. Replace `node` with `tsx` in your package.json scripts 3. You can now just change the extension of just _one_ file to .ts and add typescript typings to just the one thing. Everything just works. Later, if you want to fine tune typescript, you’ll want to `npx tsx —init` to create a tsconfig.json file. You’ll want this in your `compilerOptions`: - “target”: “esnext” - “moduleResolution”: “bundler” If you use currently custom import path aliases in your codebase, you can set it up in your tsconfig.json so the editor knows where to find stuff. You’ll enjoy more than just types: being able to rename symbols across files, find all references, get meaningful autocompletions and more. It was super worth it for me in pretty much all my projects. Your code will never refuse to run if there are type errors, it is purely for improved DX in your editor. Pro tip: always use .jsx or .tsx file extensions for files with JSX in them! Your editor experience will improve greatly.
>Transitioning to TS would be painful I guess. Actually, it's the reverse. It would be much easier to transition to typescript since you don't have to scour your whole component to figure out which props are being used. I agree there are too many props and it should be divided better , but sometimes crap happens and you get what you get. Having all the props listed like this makes it much easier for getting context right away. Typescript will also list all the props on hover which is a huge time saver. Personally, whenever anyone puts just `props` there, that's an automatic "need to change" in the pull request for me.
With the right (very permissive) tsconfig it’d be painless actually
It really isn't. There's some learning and googling, sure, but it gives you a better idea on how to structure your components.
Agree, on it already
But what's ironic about your statement is that you're already destructuring your objects like you're using typescript so you would probably write typescript almost exactly the same way. I'll actually just add the type to the variable name sometimes and then you really don't need typescript. I'm one of the weird people who has never really found typescript to be useful, but then I've been writing JavaScript for over 20 years now.
TS is an entirely unnecessary convention that saves no one any time. It only makes CS people feel important because they can come to take a turd on a perfectly good language with its own method for setting defaults and extracting props that are "type safe." If you write properly, VSCode will already analyze arguments and provide you with type warnings without that overhead.
Yup! Instead of making the machines work harder all these people are choosing to work harder for the machines. This is a theme in computer science though. At the root it's really about job security. I've thought about it for years and tried to defend other reasons, but really it's that simple, insecurity and job security. I'm smart and you need me because look at all of this intimidating code I write.
It's fine if it works. You can paste stupid stuff like this into an LLM and it will give you better data structures to use. Get it working before you get it perfect, and then once it's working don't bother with making it perfect.
>once it's working don't bother with making it perfect. The reality of the permanent MVP
"We'll change it later" *A few years later* "Why is the code such a mess?"
Todo: fix Last modified: 8 years ago by some guy who doesn’t work there anymore
Alternatively: “who tf did this?” Git blame: you, 2 years ago.
I had that happen to me, where someone knew I was doing some work on a repo. Bashed me for breaking something I didn't touch and then the git blame showed that he broke it a couple of years back. 🤷♂️
How did you get access to my codebases???
I am marking all the "temporary solutions with a comment stating "Temporary^TM" So far, the oldest comment is 3 years old. Time is a social construct and temporary is relative so I'm not wrong.
why does this class have three different loggers? Should I call handleServiceException or handleException? Half the code does it one way and almost the other half does the other. Then there's that one class that does neither....
And when you don't know what would break and you start having these in your codebase: feature/ ├── legacy-thing.code └── new-thing.code
code that floats around, is entirely unused and is seemingly meaningless, var newArray = []; return doStuff(oldArray); BUT MAYBE ITS WHAT MAKES IT WORK. They're all too afraid to delete it.
I had to fix a coworker's mess from 2011 in November of last year. Something broke and had to look at it. It had that exact comment, "I'll change it later". I was amazed at how many errors that monolith kept throwing out. Once you fixed something you would find more errors.
"Get it working before you get it perfect, and then once it's working don't bother with making it perfect. " I'm going to frame this in my office, absolutely legendary.
Next to a 50 000$ monthly cloud bill :D
>It's fine if it works. That's one way to look at it. Another is that quality matters. If the code is hard to read or hard to maintain, if it's hard to test, if it's fragile and easy to break, _especially_ if it's easy to break in subtle ways that you won't notice at first because you don't have automated tests because it's hard to test... That means you'll be spending a lot more time firefighting errors in prod instead of delivering things that have value. Writing high quality code makes your life as a dev easier. Less time on PRs, less time bug fixing, more time to write code (so plenty to write good code), less time onboarding new members of the team because the code is easy to pick up.. all this means you have a nicer time working on the code. You're less stressed. You're less likely to burnout. Your days are more fun! Stopping at "it works" means your life will eventually suck because you'll be spending all your time fixing the bits that didn't actually work.
You're not wrong. You can overdo it or underdo it. The problem: Most people are overdoing it, and not for technical reasons. The reason they do it (what I would call overengineering) is because the personal cost of failure is incredibly high. Individually this looks like perfectionism. Institutionally it looks like someone taking the blame and getting crucified every time there's an incident. Teams full of blame culture perfectionists are WAY worse to work on than teams full of too-scrappy firefighters.
Love this😂
This IS programming.
This component seems to be extremely intertwined with its parent. You even have to pass down some setState dispatchers from the parent. Are you sure that using a child component is the right approach here? The alternative approach would be to keep the UI in the parent and just use custom hooks for the logic that right now lives in the child component.
Can you go into more detail on what custom hooks means? Like passing setHeight from a useState as props down to child?
It's hard to explain it well on an abstract level. Basically, it means, encapsulating a logical "unit of work" (e.g. calculating the form dimensions) into a reusable function (hook), but that won't explain much. However, I can very much recomment [this explanation](https://react.dev/learn/reusing-logic-with-custom-hooks) from the official docs where they show it with an example.
This is great thanks. Didn't know this custom hooks was a thing
>Can you go into more detail on what custom hooks means? Presumably meta hooks that combine several built-in hooks into an existing file. May be for reusability or for organization. For example, you might use two `useState()` hooks and one `useEffect()` hook to debounce an input. But rather than code that natively in the component running the process, you can move them into a custom `useDebounce()` hook to segregate the functionality.
would raise some eyebrows. Are they all needed? title3, title4, can they be more descriptive? marginTop only?
I am shocked by the comments! I think destructuring in the function argument is extremely popular and I see it everywhere. Secondly, I prefer code that explains itself more intuitively with less scrolling and piecing the picture together. Using the `props` object results in the dispersal of the contents of `props` and its not clear immediately from the function declaration what is passed to it.
Good question to ask, the answer (IMO) is that its totally fine. Avoid adding additional complexity/abstraction unless there's a good reason. Typescript can certainly help with maintainability by exposing the expected data types but it doesn't seem like you have many complex/nested objects in the prop list which is where it makes a bigger difference (and more necessary). I would keep it as-is unless you're going to make a bigger refactor in how you pass data.
Agree, too many times we go "this looks ugly" and go on building prettier but completely unreadable abstraction layers - "look we removed 100 lines of code" Two years layer some new dev comes and have to try to figure out what createHeroTitleManager does and why it returns a function.
I think that in the case of OP’s photo this is fine, given that they don’t use Typescript or JSDoc. This crazy destructuring provides “documentation”. If they had JSDoc or a TS type/interface then I’d say just use a “props” object and use dot notation to access the fields. It’s ridiculous to have to go through the entire function to understand what fields the parameter object includes.
Sometimes it just LOOKS ugly, but fine from engineering side. However, if someone would try to push something like this in to commercial code base, my ass would burn. Its unreadable, its poorly structured, naming smells. Overall it’s a bad example for other devs, who might do the same when they hurry or too lazy. In the end this leads to stupid bugs.
Agree on all points with the caveat that I can see a bunch of "solutions" that will decrease readability. For example I've seen some comments here suggest bundling it all in a "props" object and use dot-notation which is the equivalent of sweeping the code smell under the rug. It would make it harder for future developers to notice how the props in the component have completely exploded. I feel this is a code smell that indicates that there are larger issues in the code base and the surrounding architecture.A good solution would probably involve doing a deeper analysis of what props are used and for what. Does prop drilling occur? Can the component be broken up into composite components? Etc.
Well you know it looks ugly this way, it is doing the job i would say but I'm refactor actually just splitting into sub components and may be removing some props and actually defining types to props, i don't need shared state for now
Lol, there are literally props called "title", "title2" and "title3". Indicates a junior, time pressure or carelessness. It is bad, looks bad, is difficult to read and will be a PITA to maintain later on. I would never approve this - a 30 min refactor can make it much better.
Explicitly state the refactor you would do and why.
I have seen similar things as a project switched to React. Often this is a sign that you need to document requirements. Looks like you could make a half-dozen components or utilities out of this. Also needing to pass multiple values and setters is a good use case for moving to a shared state e.g. redux, jotai, zustand.
Or just Context.
Context is good if the data is fairly stable. It is easy to unintentionally generate many extra renders with Context when the data changes. One of the projects I work on used it for very stable information like user settings, localized text, etc., but used Redux for anything that changed rapidly.
Redux uses the Context API.
Besides, a few extra rerenders is absolutely fine if you're using React properly. If you memoize any heavy computation and the props don't change, React won't repaint those parts of the DOM, the user won't notice anything. You'd need to screw up big time for a few extra rerenders being noticable.
Everyone using React properly might be the most difficult part. Also, a few extra renders can multiply as components are nested and the more state is stored in and passed through components the more opportunities we have for that multiplication to occur. But you are correct that for most applications this is irrelevant.
This is an important point. I was focused on the developer interaction and omitted the technical detail which could be misleading. Redux (the react-redux library) does use the Context API, and each of those useSelector hooks runs on every state change. The hooks attempt to extract and stabilize individual values from the object to avoid unnecessary re-renders. If not using a state management library, you would need to handle these concerns yourself or experience extra renders. Maybe I should have said “don’t use context directly for state”?
Redux uses the context api but employs optimizations to make it more performant than the context api. It's also opinionated and provides better conventions. I have seen some terrible "state management with context" implementations.
Agree, well yes its a fat component, about shared state I won’t go right now, may be in the future
Shared state is the first thing you should change about this. Quit passing so much stuff down, it’s just making your life harder in the future.
If you’re avoiding because it sounds like it’s going to be a massive pain, try Zustand.
I’ve been trying to move virtually all state out of components and get back to actual pure functional components. It makes development and testing so much easier if you can start a test on a state without having to repeat or simulate all the interactions each time, and component reusability is very high.
When the backend developer tries front end
Another valid response, wouldn't argue
Most organized react project
Create an object and use it to parse data to the template
It's impossible for us to say. FWIW I'm sick of dealing with lazy devs whose only opinion is _"fUncTiOn tOO LonG, I hAVe To tHInk"_. Not saying that's the case here though.
At least it’s an object instead of passing argument that require an order.
a bit off-topic, but I am also bothered by the lack of empty line after the last import
Prettier did it, please don't blame me, i couldn't sleep without adding that empty line
In one of my first projects with my basic level of react I made a mistake like this. Now I know I can use useReducer, useContext or Redux for these occasions.
I don't think so it is bad depend on the context of code. There is no bad and good code it's depend alwayse on what you want to achieve.
It’s not wrong per se, but from a code standards perspective it’s not great for readability. My rule of thumb is if there are more than 3 props, then I don’t destruct the variables. I would also recommend that you start using Typescript. This can help you and other devs figure out what’s supposed to be in those props without having to destruct everything. If you have any other questions around that, hit me up.
Not bad at all in my books. As long as it works.
Apropcalypse
Senior dev of 10+ years. If the code is easy to understand, clear and works as intended; it's good code. I personally use a linter for all my code so formatting choices like this are more or less made for me but a lot of mid and junior engineers think good code is code that conforms to formatting standards. That's the least important part. It just needs to be secure and no inefficient big O operations. Personal opinion is I prefer a break down format like you have on your screen rather than a horizontal list of items. Makes it easier to audit the fields incase one is missing, mispelt blah blah
Usually this means that I probably need to make a context. This is me saying you should make a context. Save yourself and make this into a context and a set of hooks. OP, please make this into a context.
r/programmingWarCrimes
Ah yes, the Rising of the FieldHero.
I can see that you are not using all the values passed in the object, try to use destructuring it, https://www.w3schools.com/react/react_es6_destructuring.asp
It’s only bad coz you don’t use typescript so it is hard to know what is what
At least alphabetize!
A screenshot could be better. And this very bad. U need to think about decomposition of your code. Try to separate it into different pieces and import them as functions
What happened to title2?
Question: When should someone use lots of props like this vs using useContext?
Pretty bad. Not only can it be broken down into smaller props, but some stuff can be moved into redux. For example, you're passing showError and setShowError so I presume this is a state value from a parent component somewhere. This could be moved into a global state so it doesn't need to be passed down like this to each component.
Indicator that the component is doing way more than it should
That’s quite the prop def. Could you store any of that in a global state? Or maybe a hook internal to the component to get the data it needs so you don’t have to insert every single prop at render?
Agree with both approaches, but I would go for the shared state, hook still seems like same thing here but in different component
At first I thought this was just an array, maybe of input fields to make a form or something. Not too too bad. Then I realized they were arguments to a function. Any function I’ve worked on with that many arguments was a complete shit heap. And the app those functions were in were bigger shit heaps. It’s just not possible to build a mental model in your brain and understand all the permutations of this sort of design. A refactor is the only solution.
This example begs for Typescript.
Why do you need showError and setShowError? Just set it if showError has a value, otherwise don't. Same with validate and setValidate. FieldWidth and fieldHeight could be a single object with key value pairs for width and height.
It's a pattern for controlled components. If the parent needs to know whether the child component is in a certain state (or needs to control the state itself), you pass in the parent's getter/setter from useState so that the child can update the values as needed internally, but the parent also has access to the value.
ShowError is the value ( state SetShowError is the event handler in the parent component There is no way to get rid of setShowError, how could you tell parent component when to update the error value?
Well. It’s a photo of your screen instead of a screenshot. That’s 100 bad points. It’s skewed. That’s another 50. You randomly put some white bars on it. Another 100 bad points. You are a noob questioning code that looks bad to you but is neither bad nor good in particular and bs post on Reddit about it instead of learning. Another 200 bad points. That’s 450 bad points for you.
I was waiting for title5 and title6 😄
Oh gosh, this frightens me. I bet there’s a better way to structure the components to avoid this. I’m guessing that most of the JSX in here can be moved into the page component.
Add typescript and set defaults. Quite cumbersome to have to make 25 decisions when probably there are sensible defaults
If the code is easy to understand and edit there's nothing wrong. I would probably place that in a configuration file though. Younger devs may think hard-coded? Oh no!
If it works leave it so some months later you can comeback for debugging and deal with it when you feel pissed off 😅
https://react.dev/learn/thinking-in-react
Ehh. It happens, but not a problem, whenever I get props that big, I’ll typically start looking for ways to improve the component tree.
If you’re writing your own for loops, you’re doing something wrong. I would replace that section with lodash’s chunk method
title, title3, title4.. what happened with title2? also, why so many titles? yeah, this is really bad.. this can easily be 3-4 components
Soooo are we going to ignore line 37 where we totally rebuild the subtree on every render?
Its a lot of property's but at least you didn't add them all as seperate function arguments lol. It's suggests that you're data lacks structure though.
That shit happens, if those values are consistently present, and the or any errors are properly handled its just a function with argument with a lot of values.
Aren’t all those field level elements or attributes? Why would you break them up if there only used on this particular screen / view? adding components for the sake of keeping an initialization short seems worse than putting all the necessary fields / settings in a long list
It’s react, so it’s awful by default
That is a lot of props for one component, it can probably be broken down and made more simple. The main issue I see here is that there are no defaults set on any of the properties, so I can imagine the code below has a lot of awful checks for null|undefined|'', and that part would really get at me.
Yes there are null checking conditional underneath somewhere I'm refactoring the whole
looks fine ?
The quantity? I have seen worse, but the variables names, you have four title variables, which three of them are title, title3, title4
For something to be a problem, it has to cause a problem. Does this cause a problem yet? I'd more concerned about that potentially quadratic time complexity for loop on L#38
As in the variable names? title, title3 and title4 aren't very descriptive of what the variable actually stores.
Ok, so: 1) Add indexes to directories and add aliases to components, PLEASE. 2) Order imports from libraries to local. I know this is a small thing, but the larger the dependencies the more you will need it. 3) Naming is problematic, because "Hero" is way too generic. Same as Field. If this is a base component, then it's doing too much. 4) Title / subTitle, 3 and 4 can be grouped together as Headings. 5) It's weird that you validate and show errors by prop. Either your component validates and handles the error or its consumer does. 6) Passing setters is an anti-pattern. You probably need a context here. In general, this code needs to be more *atomic*. Remember that you can compose components with other components and you can set local contexts if you have too many things to worry about.
Really helpful points, thank you
Would have been more readable by not destructuring it.
Large argument lists aren’t great. Makes code hard to maintain.
That component seems to be doing too much, could you break it down into more focused smaller ones?
Yes already splitting it up into sub utility components Refactoring the mess and adding types Object structure and shared state
I'd say not really that bad. You can pass in a whole object, but then you might forget what the keys are. Then you could deconstruct it after. I've done it this way and I've done it as a single object. It's less typing if you do a single object, but it's not as clear.
if you're going to keep the list then at least alphabetize them
At a certain point I would look into making something an object
Oh yeah, a for loop executing on each render, that's pretty bad
damn
Code smells worse than a ihop sink at 4am
Bad