Random Quote Machine projec JF

Hello every one!

I would like to receive an opinion from you all about my project:

I used the React Redux to develop it and it took me a LOT OF TIME :smiley:

I hope you enjoy it.

Muito bon.

The translucent card/modal thing looks nice, but it might not be the best for readibility.

1 Like

Cool. It looks pretty slick.

Putting on my nitpick hat and looking at the code…

In index.js, you don’t need a constructor. One of the things that I hate about how React is taught is that they teach you that everything is a class component and every React class needs a contructor. This:

  constructor(props) {
    super(props);
  }

is the default constructor so you don’t need it. In fact, this doesn’t need to be a class. Most modern React developers would make it a functional component, like:

const Project = () => (
  <div id="project-body">
    <React.StrictMode>
      <Provider store={store}>
        <QuoteGen />
      </Provider>
    </React.StrictMode>
  </div>
);

Of course using hooks you could write QuoteGen as a functional component too, but let’s ignore that. You also don’t need a constructor there. Most people would have written it as:

class QuoteGen extends React.Component {
  state = {
      randomIndex: Math.floor(Math.random() * quotesLength),
      randomBg: Math.floor(Math.random() * bgNumber)
  };
  animations = React.createRef();

or something like that. Then if your method is an arrow function, you don’t need to bind this:

  handleClick = () => {
  // ...

In bgAnimations.js, you are messing with the DOM. Nooooo! React should be handling that, unless that is a different section of the DOM (not typical, but possible). React should be handling the DOM. If you start messing with the DOM, you can really confuse React.

I think redux is overkill for a project like this, but I get that you want to try it out. A quick look and it looks pretty sound.

One thing that looked odd to me is:

    case GET_QUOTE:
      state = []; //Cleaning the state
      return state.concat(quotes[action.randomIndex].quote)
        .concat(quotes[action.randomIndex].author);

Why not just return the object as the new state??

    case GET_QUOTE:
      return quotes[action.randomIndex];

I might also say that you’re doing the same random gen in a couple places. I might have just made avoidRepeatedNum into getRandomIndex and made the oldNum the second parameter as optional and let that function handle the logic - usually I try to put logic as deep as I can, to hide it.

Also, I’m not sure what the value of this is:

export default function avoidRepeatedNum(_oldNum, _maxNum) {
    const oldNum = _oldNum;
    const maxNum = _maxNum;

Why import the variables and copy them? Why not just:

export default function avoidRepeatedNum(oldNum, maxNum) {

Usually you prefix variables with an underscore to tell people that you consider them private to the module. There is no danger here because they only have scope in this function so no one has access to them.

Still, it looks pretty good. This is tricky stuff and has a certain irreducible complexity. But you’re doing some cool stuff and are on your way.

2 Likes

Hi @CactusWren2020!

Well, I agree with you, but I think a translucent card gives the page a nice detail and makes possible for the user to see the image behind it.

I was using this as a background color:

background-color:hsla(0, 0%, 100%, 0.5);

After your comment, I changed it to:

background-color:hsla(0, 0%, 100%, 0.6);

And I think the page looks better now! Do you agree?

Thanks for your comment! :facepunch: :facepunch:

I don’t remember! But it looks pretty readable now, so probably :slight_smile:

1 Like

Hi @kevinSmith!

About the constructors, I removed them all and I see no difference on the App’s working :sweat_smile:

And about it, I think I should review some lessons, because I really didn’t get the meaning of this structure.
I changed the Project component to a functional stateless one, I agree that there is no reason for it to be a class stateless one :grin:

About it, could you be more specific about how did I mess the DOM? :sweat_smile:
Did you mean that I should use: React.createRef() instead of document.getElementById()?

Answering your question: in the beginning of the project I thought that the reducer should always return the state, but then I realized that this is not true :sweat_smile: :sweat_smile:
And I agree that was more direct return an object instead of an array
I made this change on my project.

At this point, I was trying to use some concepts of Functional Programming, so then I wrote functions on that way to avoid making some change at the input variables. In my head this is a good programming practice, don’t you agree? :grin:

And finally, thank you for the complements, I am glad that you enjoyed my project!
And I would like to thank you again to take some time of yours to take a look at my code and make a lot of significant comments on it, I learned a lot from you! :facepunch: :facepunch:

Did you mean that I should use: React.createRef() instead of document.getElementById() ?

I don’t know if you have to use refs - which should be a last resort, but you should be using a React solution. React should be the one changing the DOM.

Answering your question: in the beginning of the project I thought that the reducer should always return the state, but then I realized that this is not true

Well, it should always return the state. But in this case, your reducer state is just a simple object with the quote and author information. You don’t have to “clear the state”. In fact that would have no effect on the state. Whatever you return from the reducer, that completely overwrites the reducer state. You always return a new version of state and that becomes the new state, whatever you return. Or (usually by default) you can return the old state.

At this point, I was trying to use some concepts of Functional Programming , so then I wrote functions on that way to avoid making some change at the input variables.

First of all, those are primitives so they are sent by value, so you can’t change the original, even if you wanted to. And if they were sent by reference (object, array, function, etc), then all you would be doing is copying the reference so it wouldn’t have the effect you think it would - you would still be referring to the same place in memory, you’d just now have two references to that same one place in memory and either reference will affect the memory to which the other point - because it’s the same block of memory.

So, you never need to do this with primitive types. And there are situations where you might want to do this with reference types, but this isn’t how to do it. You either have to clone the object or just use it in a way that doesn’t alter the original.

But this is a complicated and confusing subject if you haven’t done a low level language where you have to manage and be aware of how memory is used. The great/horrible thing about JS is that it does this for you behind the scenes so a lot of it is hidden from you. I’d recommend seeking out some tutorials on JS reference variables.

1 Like