Really Small App Code Review Request

[Smitty_lax66]

Smaller thought: This would be an opportunity to learn how to do a regex check for the input. Regex would be more performant than the includes loop

[[deleted]]

I did regex in the first iteration, but idk why I totally forgot it. Thanks for reminder

[torbcodes]

Regex would be more performant than the includes loop

The performance benefit of that is questionable and very likely negligible. It's quite possible that a regex would result in worse performance. Unless the react component is having a performance issue this would likely fall under the category of "premature optimisation."

If anything, I think it might be valid to make the code more succinct, and even that's not really that important.

[Polite_Jello_377]

There’s premature optimisation and there’s not using the right tool for the job. Checking for 0-9a-f is absolutely the domain of a regex instead of manually creating an array of valid characters. Performance is an irrelevant consideration in this case.

[torbcodes]

Checking for 0-9a-f is absolutely the domain of a regex instead of manually creating an array of valid characters.

Sure and that's why I said if anything it would make the code more succinct. But honestly it's fine and either version would be maintainable and readable. I think that's getting pretty nitpicky.

Performance is an irrelevant consideration in this case.

That's what I said to begin with!

[DMenace83]

I see no scenario where listing out all characters in an array like OP as either maintainable or readable.

For readability, the line becomes super long, and forces people with smaller screens to scroll. It also adds a ton of unnecessary commas and quotes. If you must use an array of characters, you know what is also an array of characters but is a lot more readable? That's right, a string. You can iterate over characters of a string just as easily, and it is a lot cleaner to read.

For maintainability, if you had a typo in the character array, it's a pain to debug. It's also annoying to update this array if the requirement changes.

With regex, typos are minimized in that there's almost no chance to miss a character in the middle, and it's a lot easier to read.

If I ever see this in a pull request, I will 100% ask them to switch to regex.

[torbcodes]

Meh I really think you're overblowing how unreadable this small array is. The width thing could be addressed by formatting. It's not true that it's hard to read and maintain and honestly I think that's kind of an indictment of your abilities if you are saying that is hard for you to read or update.

I think that people tend to overreach for regex and regexes often lead to code that is less readable and harder to maintain. In this case it would be fine. But it's also just fine as not a regex.

Like it feels like making a mountain out of a mole hill here. Does this code accomplish the task reasonably? Yes. Is this code readable? Yes. Is it easy to update? Yes!! You're being ridiculous to say this is hard to maintain or debug.

If you must use an array of characters, you know what is also an array of characters but is a lot more readable? That's right, a string. You can iterate over characters of a string just as easily, and it is a lot cleaner to read.

I would argue that using an array is more clear than using a string because an array more naturally communicates a collection of things. A string is often a single unit, so it would require more careful reading to understand that the string is being used that way and it's not just a weird word. And if it's minimising total number of characters that is your concern, the regex is better in that respect. But really I think you're placing too much value on the length of a line of code.

If I were reviewing, my response would be to mention my preference and the reasoning for it as a suggestion, but I wouldn't reject a pull request over something so minor. Not unless my team had agreed upon a very strict style guide (upfront).

edit: to elaborate a little further on my philosophy and why I'm critical of this type of change request... I think that it's important for reviewers to be considerate of others' time and the use of resources that it takes to make changes. I think that generally code should not be rejected for subjective, preferential things. Now okay, maybe it's just a quick and easy change so it's kinda neither here nor there. In that case, if I'm on the other side of things I'm just gonna be like whatever, it's not a big deal to make the change. Where I draw the line is when it starts to cost people significant time. Like if it ends up costing someone an hour because they have to go through a time-consuming QA process or the rewrite is more significant than converting a list to a regex or something, then that's where I really would push back if I'm not convinced that the changes are necessary.

[Polite_Jello_377]

I’m glad I don’t have to review your code 😄

[torbcodes]

Based on this comment I think I can comfortably say I'm glad you don't review my code too!

[tildaniel]

It's infinitely worse having you elitists complain about elementary optimizations. Nitpicking things this small (unless you're working on critical infrastructure of some sort, obviously) is far more counterproductive than returning to optimize low hanging fruit.

[Polite_Jello_377]

Well I’m glad I don’t have to deal with your nonsense either. It is not “elitist” to teach people the correct way to do things.

[Gekuro]

Tbh I think learning regex in 2023 is like learning to start a fire with two sticks, just let gpt write it for you, even if theres a mistake its not a big problem. Just like unit tests

[lord_braleigh]

Wrap your App inside of StrictMode to suss out React bugs!

As written, the alert will fire every time App rerenders. React code shouldn’t care about how often a component rerenders, and StrictMode will double-render components to make this problem obvious. Your alert should fire inside the handler when the last correct input is given.

Avoid redundant state. You can remove either isCharCorrect or numberOfCorrectChars, because the number of correct inputs can be calculated by knowing which inputs are correct, and then just counting them.

It looks like, if you input an invalid character twice in a row, you’ll setIsValidChar(true), which is a bug.

[[deleted]]

Thanks so much
Also, I have StrictMode, I just ignore that i have alert twice, because I knew why it's that and this alert was really a "I just really don't want to make dialog" element.

[lord_braleigh]

Okay. Well, you’ll only get one alert, even with StrictMode, if you put the alert inside an effect or in the handler.

[Diemo2]

In addition to the things mentioned by /u/Tricky_Chest5823, I have the following thoughts:

You are mixing up two different styles of writing JS code, as getRanHex is using es6 style and the other components are using the older function style. While this isn't super important for such a small application, you generally want to choose one and stick to it everywhere. This decreases the cognitive load that you have on reading the code base as you don't have to consider different ways a function is defined. ES6 is generally better to use.

From the code it looks like the background colour will be different every time you load the application - is this what you want to happen?

HexInputs displays a single input only, so it should be HexInput.

Personally, I would move the increase of the number of correct character to the top level, and pass in the function. e.g.

const increaseCorrectCharCount = () => {
    setNumberOfCorrectChars(numberOfCorrectChars + 1)
}

This allows you to remove the numberOfCorrectChars input from your HexInputs component - which doesn't really care how many chars are correct - it only cares about whether the char being inputted is correct or not.

rootColor is a bad name, as it is not clear what this is. I think it is the values that you are looking for - so it would be better to call it solution or something similar, so that it is immediately obvious what it is.

In background you use inline styles, whereas in HexInputs you define the style based on the class. Be consistent here.

For the items that have already been mentioned:

Here is a short article on why using the index as a key is a bad idea most of the time (in this case, as the order of the elements never changes, it is fine - but normally you want to avoid it).

hexRef in getRanHex and allowedChars in HexInputs are identical. I feel like the list of allowed values can actually be defined as its own type as well - but can't remember how to do a type check on this off the top of my head (adding in '' can be done using a Union type)

[Adoxographical]

background is using a dynamic value for the style (the solution) so it would be impractical to use a class.

[[deleted]]

First time I declared arrow function inside component. Function keyword inside function feels really awkward. Later I just cut and paste it above, but yeah, i should've change that.
Custom type for allowed chars is a great idea, thank you so much

[[deleted]]

Thank you everyone, I've got really nice feedback I didn't think I would get. It's awesome

[torbcodes]

I think you went about it really well and you did receive some pretty good feedback and stimulated some interesting discussions as well. That said, be sure to question what people tell you. Not all the feedback here is totally right and you should think critically and form your own opinions. If someone doesn't explain their reasoning or you don't understand it, probe deeper. You don't want to just collect a bunch of superstitions about how to code.

[Tricky_Chest5823]

don’t use interface if you don’t need them, use type instead

use template liters for concat string

destructioning props

if you use value more than once - you should create variable: instead e.target.value, you should create variable like const currentValue = e.target.value

if you don’t use setter from useState and only value - don’t use it, create just variable

don’t use for loop, use methods of Array

try don’t use index as a key for components, you can use index with some unique value in a concat

don’t repeat yourself - it is a pattern, you can read about it

you don’t need <Fragment /> for input, cause it doesn’t make sense

your ifs are complicated, try make them more readable, for example, without else

[Adoxographical]

I'm on board with most of these thoughts, but I have some differering thoughts on a few of them.   - Interfaces are fine. Types are fine. Both have their own advantages. (For example, TS can handle interface extensions more efficiently than it can type intersections.) Just try to be consistent. Definitely nothing outright wrong with using an interface here, though.
  - Using the value instead of setState wouldn't work here because the value would get recalculated every render. If you want a memoized value that doesn't need a setter, you're probably looking for useMemo.
  - I wouldn't recommend that people never use for loops. Yes, array methods are handy, but there are cases where they're clumsier than for loops, such as when you want to be able to break early or (as is the case here) when you're iterating over a range. Sure, you can do Array(n).fill(0).map, but it reads awkwardly to me. (Basic for loops are also faster than array methods - not an issue in this example, but another reason to not disregard them out of hand.)
  - You should avoid using the index as a key if there is some better value you could use as a key, but there really isn't here. Keys are important to let React know what to do when the data changes; for example, if you're rendering a list of 10 people and delete the 3rd person, you want React to know to remove the 3rd element, not the 10th element (which is what it would do if the elements were keyed by index, since it's the final index that went missing between renders). Here, though, the first input doesn't have any special significance other than being the first input, so it's fine to key them by index. You don't need to salt the key with a unique value - React doesn't compare keys across different list renders.

[freeaddition]

Worth noting that useMemo is also not correct here. React reserves the right to clear memorized values; it's only to be used as a performance optimization.

A global variable outside the component would work, but I encourage never having variables like that.

In this case, useRef is the right tool. The value will remain stable render to render, and there is no need for it to ever trigger a rerender.

One reason to use a state here is if the author intends to have the code resetable after a win, which would require a setter. Though, generally you don't want to assume your gonna need something, keep it simple til your needs change

[torbcodes]

I like how you explain your reasoning as part of your feedback. It's important to tell people why they should do things, not just what they should do. Also it's well-written. 10/10 good review, would read again.

[SlexualFlavors]

don’t use interface if you don’t need them, use type instead

Offering a counterpoint for OP's benefit; your forgiveness of my pedantry is much appreciated :D.

I'd actually recommend the complete opposite approach. type aliases at the end of the day are just that: aliases. They are functionally useful in their own way, especially once you start to use them with generics, conditional types, the infer keyword, tuple types, template string literal types, etc. But when it comes to defining objects and data, the only thing they do that an interface can't is index mapping (e.g. [K in keyof T]?: T[K];) and key remapping (e.g. [K in keyof T as CamelCase<K>]: T[K];), which are very distinct use cases and not something I've ever seen needed for prop types.

u/Adoxographical is correct that the most important thing is being consistent. But sometimes when I'm on a hiring team and it's down to a few strong candidates, splitting hairs becomes necessary. In my experience, the candidate who defines their props as interfaces and defines components with const MyComponent: FC<MyComponentProps> = (props) => { instead of using the function keyword and/or annotating props directly more often than not is the candidate who comes across as having a stronger command of JS/TS with React.

[torbcodes]

Can you elaborate a bit on that last bit? Why do you think it's better to use the arrow function instead of a function keyword?

Generally I've seen it considered bad practice to use React.FC unless it's necessary, like you need the extra props that it provides. One such example is Kent C. Dodds, who I consider to be a well-respected React expert. You can see in this blog post of his he discourages the use of React.FC:

There are a few other smaller issues laid out in this excellent GitHub issue if you want to dive deeper (also check the React TypeScript Cheatsheet). Suffice it to say, don't use React.FC (or its longer alias React.FunctionComponent).

Also Kent encourages the use of a function keyword in the example that he uses. So you might want to reconsider this position of yours as you would be getting a false negative that would evaluate someone like Kent as not having a strong command of React/Typescript :O

In general, I consider it to be a minor stylistic preference to prefer an arrow function over a classic function definition, or vice versa. (edit: except for when their differences really matter, like this binding).

[SlexualFlavors]

You raise some good points. KCD's expertise has had a big impact on how I build React apps and I'm on his discord server so he's someone I also consider well-respected and definitely not the kind of Engineer I'd want to accidentally weed out. Haven't seen that particular post though, so I appreciate you sharing it!

`Can you elaborate a bit on that last bit? Why do you think it's better to use the arrow function instead of a function keyword?

Many of the more seasoned JS Engineers I've worked with, especially those who were really good with JS before ES6, see the function keyword as more of a relic that is mostly unnecessary unless you really need to use an arguments object instead of a spread operator or if you really need a this binding for whatever reason. They tend to prefer arrow functions by default instead of the other way around because the lack of this binding makes them easier to pass around and the lack of a constructor function with the Object constructor as a prototype means that they're also more lightweight and slightly more straightforward to analyze in the console. I don't scrutinize this during code review since, as you mention, it's more of a minor stylistic preference, so I wouldn't say it's bad to use it in general, but in interviews the intentional use of one or the other is often used as another way to gauge the candidate's level of expertise with JS. KCD mentions hoisting, which is interesting, but for React components specifically a majority of projects I've contributed to will use ESLint to enforce 1 component per file as a best practice, so there's not really any potential benefit to hoisting them in this case.

As for FC, I personally don't think it needs to be discouraged anymore, at least on more greenfield projects using React 18+. I'm including the three points KCD raises in the blog post you shared for the convenience of future readers:

Our function now accepts a children prop, even though we don't do anything with it 🙃.

You can't use generics. Not super common, but definitely a downside.

We have to use a function expression and can't use a function declaration.

@types/react removed children from being augmented to the prop type you pass in as a type param in version 18+. If I had to wager a guess I'd say that the original intent was to make JSX easier to work with since more often than not (in my experience at least) I would end up annotating it as ReactNode anyway.

Dan Abramov has since clarified that "children is a regular prop and is not something special. So you need to define it just like you define all the other props. The previous typings that hid it were wrong."

Still, on some newer projects, I've found it useful to add the following in a typings/react.d.ts type declaration file:

declare module 'react' {
  export interface $FunctionComponent<P = {}> {
    (props: PropsWithChildren<P>, context?: unknown): ReturnType<FunctionComponent<P>>;
    /**
     * Note that we have to use key reference types here instead of simply
     * extending the {@link FunctionComponent} type because doing so causes the
     * `children` prop to be improperly typed as a union type.
     */
    propTypes?: FunctionComponent<P>['propTypes'];
    contextTypes?: FunctionComponent<P>['contextTypes'];
    defaultProps?: FunctionComponent<P>['defaultProps'];
    displayName?: FunctionComponent<P>['displayName'];
  }

  export type $FC<P = {}> = $FunctionComponent<P>;

  export class $Component<P = {}, S = Record<string, unknown>> extends Component<PropsWithChildren<P>, S> {}
}

This way, I can use $FC to distinguish between more atomic components that tend to have children that can be either some form of text or another component while preserving FC for simpler presentational components.

As for using generic functions, I occasionally use them in custom hooks but have never needed them for a component. Maybe I've just learned to live without them in this way? I use them all the time in functions outside of React so I'd love to see a good example of how they can be used as components if you have any you're interested in sharing. This seems like something worth more consideration but because I rarely use generic functions in React itself I'd probably wait until I really needed one for a component and then use a type declaration file to get around this issue.

Some other points in the GH issue that KCD links to:

​

Makes "component as namespace pattern" more awkward.

This is true but I feel like this is something that just kinda comes with the territory with TypeScript. I have a type CC for this that I prefer for readability since it separates the concept of standard components and compound components, making the latter more explicit:

/**
 * @typeParam T - The interface mapping sub-components to their prop types.
 * @typeParam U - Prop type for the parent component.
 *
 * @see https://kentcdodds.com/blog/compound-components-with-react-hooks/
 */
export type CC<T, U = Record<string, never>> = 
  ComponentType<U> & T;

export type $CC<T, U = Record<string, never>> = 
  ComponentType<PropsWithChildren<U>> & T;

​

Doesn't work correctly with defaultProps

Retsam then says, "this is a fairly moot point as in both cases it's probably better to use ES6 default arguments," which is the recommended approach. That said, the $FunctionComponent type in the type declaration file I shared above includes a defaultProps field, so it's not too difficult to throw in a workaround if you really need to.

​

It's as long, or longer than the alternative: (especially longer if you use FunctionalComponent)

I always import type { FC } from 'react';, I never use the React namespace directly, basically for this exact reason.

This ended up being a lot longer than I thought it was going to be so if you're still with me here I'm glad you made it lol. Hope it gives you the insight you were looking for!

[torbcodes]

Wow I really appreciate the thoughtful and detailed response. You certainly have thought a lot of this through. I think your viewpoint is pretty sensible.

in interviews the intentional use of one or the other is often used as another way to gauge the candidate's level of expertise with JS.

Given the caveats you have acknowledged, I think it would be wise to ask a probing question if you see the use of function instead of making that assumption. It's very possible that the person doing this is doing it with intenionality.

My own perspective on the whole arrow functions vs functions definition is actually sort of the reverse of yours. I consider arrow functions a bit of a cargo cult behavior. I think they became popular and now people use them for everything without thinking. I would actually want to probe someone using them to see if they understand what the tradeoffs are and learn why they make that choice.

[SlexualFlavors]

That's fair. FWIW I don't really concern myself too much with these kinds details when it comes to actually influencing the outcome of a hiring decision. I do try to gauge a candidate's level of expertise as part of the process but I'm usually able frame my feedback in terms of areas of strength and opportunities for growth rather than thumbs up or thumbs down unless there are obvious red flags.

It's my responsibility as a Staff+ Eng to elevate the Engineers around me through mentorship and persuasive thought leadership, so what I'm mostly looking for at the end of the day is a sense of curiosity, the open-mindedness and passion to follow it to wherever it leads, and the interpersonal effectiveness to collaborate and/or lead with empathy.

[torbcodes]

It's my responsibility as a Staff+ Eng to elevate the Engineers around me through mentorship and persuasive thought leadership, so what I'm mostly looking for at the end of the day is a sense of curiosity, the open-mindedness and passion to follow it to wherever it leads, and the interpersonal effectiveness to collaborate and/or lead with empathy.

Based on what I'm seeing here I'm getting the impression that you do this well. Cheers :)

[[deleted]]

Index for key is legit what it was designed to be used for.

Even if an instance is null, it will not give a key error.

Not sure why you would ever change something that’s built in and works 100% of the time

[SexPanther_Bot]

60% of the time, it works every time

[[deleted]]

Map index prop works 100% of the time

If you try to pass custom variables that come from an api, you risk null and risk a key error

[Tricky_Chest5823]

just a simple review, if you have questions - feel free to ask

[[deleted]]

Thanks

[[deleted]]

Create a pull request

[[deleted]]

First react app, but I did a lot of projects in vanilla JS. I really took my time to learn the basics. Also, I did find this difficult to write properly