Hi there,
Im appreciate if anyone can provide feedback for my 1st react project. I finished this with react and bootstrap. For the react part, i have created a parent component with many child components, it looks a lot of typing . Do you suggest to identify each component like this.
project link: https://codepen.io/kaheng-lei/pen/YzLavGm?editors=1111
Right, but that’s the React way to do it. In the long run, it’s a little extra typing, but organizes things much better. In a “real” app, all those components would be in separate files, in a logical folder structure. It’s much easier to understand and maintain.
OK, keep in mind that I’m very, picky, opinionated, and am fairly fast typer…
Taking a look at your code…
The formatting is OK but could go better. In a “real” code editor, there are tools that can help. For now, in code pen, in the upper right corner is a pull down menu and you can select “Format JavaScript”. It makes it a little easier to read. But, what you had was pretty good.
All in all I can’t complain about much. A few little nitpicks could be:
const content = this.props.quoteList[this.props.randomIndex]["quote"];
why not:
const content = this.props.quoteList[this.props.randomIndex].quote;
You only need bracket notation if the property is in a variable or is not a valid JS identifier (alphanum, _, $, and can’t start with a number).
<div id={this.props.id}>
<h1>" {content}</h1>
</div>
Did this need to be wrapped in a div? I think you could have applied whatever styling, ids, whatever to the h1. It keeps your DOM from growing out of control. I mean, if you need it, you need it, but if you don’t then lose it.
class Wrapper extends React.Component {
constructor(props) {
super(props);
this.state = {
randomIndex: 0,
quoteText: "",
quoteAuthor: ""
};
this.handleNewQuoteClick = this.handleNewQuoteClick.bind(this);
}
handleNewQuoteClick(newRandomIndex) {
this.setState({
randomIndex: newRandomIndex
});
}
// ...
Part of this is just how FCC teaches React - everything is a class and every class has a constructor. Component has a default constructor so you usually don’t need one for a component. You can declare state directly as a class property and if you use an arrow function, you don’t need to bind this (arrow functions don’t create their own this) so this could be simplified to:
class Wrapper extends React.Component {
state = {
randomIndex: 0,
quoteText: "",
quoteAuthor: ""
};
handleNewQuoteClick = newRandomIndex=> {
this.setState({
randomIndex: newRandomIndex
});
}
// ...
That’s probably how you will see it in the “real” world. Actually, in reality nowadays people aren’t writing class components much because now functional components can have state and their own version of lifecycle methods (through hooks). Don’t get me wrong, it’s really important to learn class components. I don’t write class components anymore, but I do have to maintain them in older code.
return (
<div>
<div id="quote-box" className="well">
<QuoteBox
quoteList={this.props.quoteList}
randomIndex={this.state.randomIndex}
/>
<div className="bottom">
<Tweet
id="tweet-quote"
quoteList={this.props.quoteList}
randomIndex={this.state.randomIndex}
quoteText={this.state.quoteText}
/>
<NewQuote
id="new-quote"
quoteList={this.props.quoteList}
randomIndex={this.state.randomIndex}
onNewQuoteClick={this.handleNewQuoteClick}
/>
</div>
</div>
<div className="codeBy">
<a
href="https://www.freecodecamp.org/KaHengLei"
target="_blank"
id="codeBy"
>
by KaHeng Lei
</a>
</div>
</div>
);
If you are just using a div to group nodes so you can return a single node, consider using a fragment. It will group them but then “dissolve” in the DOM, like it isn’t there.
return (
<React.Fragment>
// some node
// some other node
</React.Fragment>
);
or if your version is high enough:
return (
<>
// some node
// some other node
</>
);
Actually, it looks like it does affect the centering a little. I don’t know if it is worth it, to have to fix another problem. But in some cases, components that are rendered hundreds of times, it can make a difference.
I don’t know if “Wrapper” is a good name for the main component. “App” is traditional.
In NewQuote, you have:
handleNewQuoteClick() {
const newRandomIndex = Math.floor(
Math.random() * this.props.quoteList.length
);
this.props.onNewQuoteClick(newRandomIndex);
}
which ultimately calls:
handleNewQuoteClick(newRandomIndex) {
this.setState({
randomIndex: newRandomIndex
});
}
So, the logic of creating a new quote is split between two methods in two components. To me, the component NewQuote should just handle the click and declaratively say that new quote is needed. I think the button should call this.props.onNewQuoteClick directly. It should not pass an index - it does not be interested in how a new quote is created (imperative programming), it should just, like Caesar laying on a fluffy couch, being fanned with ostrich plumes, say, “Bring me wine, thrall!”. He doesn’t tell the slave how to get to the wine cellar, how to open the bottle, etc. This should be declarative - just say what you want. Let the part of code that handles the quotes deal with the specifics of how to get a new one.
And then all that logic should be in that method Wrapper since that is storing/handling the quotes. That handleNewQuoteClick in Wrapper should have all the logic - create the random number, set the state. And I don’t think it should be called that. It doesn’t care about clicks. It’s job is to get a new quote - it’s name should reflect that.
In theory, in a more complex app, you might have separate functions to generate the new data and a separate function to update the state/DB/whatever - in that case I’d have a third function to wrap those and pass that.
this.state = {
randomIndex: 0,
quoteText: "",
quoteAuthor: ""
};
It looks like you are only using the random index - you don’t need the others. Good, that is a better way to do it.
I think “randomIndex” is a bad name. It tells me something it already assumes and doesn’t tell them something they need to know. I would call it “quoteIndex”. In a large program you may have lots of indices.
Also, when passing it:
<QuoteBox
quoteList={this.props.quoteList}
randomIndex={this.state.randomIndex}
/>
Don’t send the entire list and the index. That component shouldn’t care about how the quotes are stored. All it needs is a quote. I would send it:
<QuoteBox
quote={this.props.quoteList[this.state.randomIndex]}
/>
Just as a reminder:
<NewQuote
id="new-quote"
quoteList={this.props.quoteList}
randomIndex={this.state.randomIndex}
onNewQuoteClick={this.handleNewQuoteClick}
/>
if we rework that click handler, this would just be:
<NewQuote
id="new-quote"
onNewQuoteClick={this.handleNewQuoteClick}
/>
and we normally don’t put the id directly on the React component like that, you’d put in in the JSX. I see that you’re passing it in getting it off props. There is no need to do that - this is only one NewQuote so you can hard code in on the div. The only time I’d do what you’re doing is if I had a component like say MyButton and I had different versions of that that needed IDs so I passed an ID in with it. That would make sense for that situation. Here it is wasted typing and complexity.
Looking at the app…
You only have an icon for the tweet? What if someone is using a screen reader? How is a web crawler supposed to know what that is?
Something other things to consider…
Again, getting very nitpicky, but you can get the same quote two times in a row. Is there a way to prevent that? I can think of two ways, one sloppy and one elegant. I’m not saying you need to do it, but it is something to think about. Part of being a developer is thinking about all the possible angles.
Your app starts up with the same quote every time? Is that good? If you had an app on the iPhone store and it gave the user a random inspirational quote, but it was the same first quote every day, every time you opened the app…
Still, it’s a great job. It’s probably better organized than my quote machine was.
As the the observations, I’m not saying you need to make any of those changes, their just things to consider for when you make your next app. Have fun on the Markdown Previewer.
Thank you so much for providing this useful feedback. As a beginner, its important for me to know how things works in the “real” world. Your suggestion helps me to simplify the coding process and also the logic.
I have rewritten this project using functional component instead of class component. It took me sometime to figure out how useState works. However, the revision works in Visual Studio Code but not in code-pen. I had all the elements with the required ID name.
project link: https://codepen.io/kaheng-lei/pen/YzLavGm?editors=1111
Your app starts up with the same quote every time?
I gave the initial quote index a function of creating a random number in the useState.
Again, getting very nitpicky, but you can get the same quote two times in a row. Is there a way to prevent that?
maybe create a variable to store the previous quote index, and do a comparsion between the current and the previous?
Cool, I’ll take a look when I get a chance. And again, don’t get me wrong, it is really important to learn class components.
maybe create a variable to store the previous quote index, and do a comparsion between the current and the previous?
Yes, that would be the messier approach. Just keep generating new numbers until you get a new one. Of course, in theory it could take several tries. Yes, in practice, especially in the n is large enough, it would be rare… but there is a clever way to do it where you will always get a new number.
but there is a clever way to do it where you will always get a new number.
how about incrementing / decrementing the generated random index by 1 or any number if the previous index is equal to it?
I’m not sure if I understand how that would work…
Think of it this way. You have a standard 12 hour clock. You have to put a marker on an hour. You have to randomly move it to a new hour that cannot be the old one - how would you do that?
create an array that contains the rest indices (not including the old one). Then pass this array to the random() function that will return a random index (for example: x).
Array[ x ] will be the next random quote index.
The way React is taught at FCC, where everything is a class and every class has a constructor, contributes to some of this. You typically don’t require a constructor for a component because it comes with a default constructor. This could be abbreviated to: You don’t need to bind this if you use an arrow function because they don’t construct their own this. You can specify state directly as a class property.
That would work, but there is an easier way. There is no need to build any other data structure. You just need to get a random number (hint: It’s base is not 12.) and a little math. The circular nature of the clock is itself a hint.
I looked at your FC version of the app.
I had to change the id that ReactDOM was targeting to get it to work.
I don’t have to much to complain about.
I might have wanted to see thing broken down in more components. I mean, I get it that this is such a simple app, but it’s important to use these are training tools for how you would break things up.
I don’t understand that you define this function:
const randomQuoteIndex = () => {
return Math.floor(Math.random()*(props.quoteList.length))
};
And then you repeat that code here:
onClick={()=> setQuoteIndex(Math.floor(Math.random()*(props.quoteList.length)))}
Why not use your function:
onClick={randomQuoteIndex }
There is an important acronym in coding - DRY - look it up.
I am thinking using Math.floor(Math.random()*(max-min + 1) + min), the min is the old number, but this will miss out the range of 0 to min, so i think it doesn’t work.
right now i can do is:
const newNumber = Math.floor(Math.random()*length)
if (newNumber == oldNumber ) then newNumber +1
but there is a bug when the oldNumber is the max, so max + 1 is out of the range.
can i get more hint…
thank you so much for checking my work again.
OK, continuing on, with our clock metaphor, if I have a random number on the clock, and I want a new random number on the clock, let’s say I imagine moving clockwise around the clock from my current number. What are the possible number of moves? What is a range of numbers I could select that would ensure that I got a new number and that all other numbers were equally possible (in other words, random)?
1 to 11 moves. and the base should be 11. sorry i still dont get it. can you tell me the answer?
If I have a 4, I want to generate a number between 1-12, excluding 4 - 11 possible numbers. If I generate a random number between 1-11 I can “move” around the clock from there. In other words, I can add it to 4. Of course, if I get a random number of 10, I have a problem, there is no 14 on the clock. I need to do the mathematical operation of…
newNumber = Math.floor(Math.random()*(max-1) + currentNumber
if (the newNumber > max ) then return newNumber - max
using the clock example, first i got 4, then generate a number between 5 to 15 (11 possible numbers), if i get 15 which is over 12, it will be 3.
Sure, that will work. I was thinking to generate a number between 1-11 and add it to the current, but generating a number between 5-15 is the same thing.
This:
if (the newNumber > max ) then return newNumber - max
I was thinking of a modulus - I think that would be a little cleaner, but the end result is the same.
So, there you have it - how to generate a random number in a range that doesn’t repeat the previous. This solution is better because it doesn’t use a loop.
This topic was automatically closed 182 days after the last reply. New replies are no longer allowed.