Code review - React

Code Feedback
1

Hi!
My project is made with React, BEM, Bootstrap. I’m asking for code review. Every feedback is valuable :slight_smile:

2

Cool, I’ll give it a go. I’m mainly just looking at the React. For some reason, I can’t get the app to start.

First of all, things look generally good. You have things logically organized and things look pretty clean.

One observation is that a lot of these components can be functional components. I find it a little frustrating that they always teach React with everything being a class and having a constructor. The only time you really need a class is if you need component state or a lifecycle event - and now with hooks you really don’t ever need a class. But the components without state of lifecycle events - those should be FCs.

In RodoText.js, as I was mentioning, you really don’t need a constructor. A log of people would write:

class RodoText extends Component {
  constructor(props) {
    super(props);
    this.state = {
      showRodo: false,
    };
  }
// ...

as

class RodoText extends Component {
  state = {
    showRodo: false,
  };
// ...

… just setting the class property directly.

In Form.js - this is the file that gave me the most trouble…

Again, I wouldn’t have a constructor. I would have declared that large initial state up above in a constant and then used it to set the state property directly on the class. And you don’t need to bind this if you define your methods as arrow functions, like:

  validate = () => {

And I hate the function validate. I’m not really sure about how you’re managing your validation, but we could at least clean a few things up. First of all, anytime you have something along the lines of if-true-else-false, you can usually simplify. For example:

    if (this.state.name.length === 0) {
      this.setState({ nameInvalid: true });
    } else {
      this.setState({ nameInvalid: false });
    }

You are doing the same thing, just with a different value. Why not replace those 5 lines with:

this.setState({ nameInvalid: this.state.name.length === 0 });

And length === 0 always seems a little silly to me, a lot of people would do:

this.setState({ nameInvalid: !this.state.name.length });

For that matter, may not put those _setState_s together? The whole method can be:

  validate() {
    this.setState({
      nameInvalid: !this.state.name.length,
      streetInvalid: !this.state.street.length,
      emailInvalid: !this.state.email.length,
      cityInvalid: !this.state.city.length,
      numberInvalid: !this.state.number.length,
      postInvalid: !this.state.post.length,
    }, this.send());
  }

I’m not sure how your onChange handlers work. (Again, I couldn’t get the app to run.)

            onChange={() => this.setState({ name: event.target.value })}

What is the event being passed in there? I would expect that to come from the element, expecting something like:

            onChange={ event => this.setState({ name: event.target.value })}

For that matter, you do this so many times, I might make a helper method like:

  handleOnChange = (event, field) => {
    this.setState({ [field]: event.target.value })
  };

Then you can use it like:

            onChange={ event => this.handleOnChange(event, name) }

Or you could get fancy and do a currying function to make it even cleaner:

  curryHandleOnChange = field => event => {
    this.setState({ [field]: event.target.value })
  };

and call it like:

            onChange={ this.curryHandleOnChange(name) }

Or something like that - I’m just doing it by eyeball.

And in Donations.js, changeDonation should not be:

  changeDonation(donation){
    if (donation===50){
      this.setState({ button50Active: true });
      this.setState({ button70Active: false });
      this.setState({ button100Active: false });
    }
    if (donation===70){
      this.setState({ button50Active: false });
      this.setState({ button70Active: true });
      this.setState({ button100Active: false });
    }
    if (donation===100){
      this.setState({ button50Active: false});
      this.setState({ button70Active: false });
      this.setState({ button100Active: true });
    }
  }

but simply:

  changeDonation(donation){
    this.setState({
      button50Active: donation === 50,
      button70Active: donation === 70,
      button100Active: donation === 100,
    });
  }

But even then - I think the donation should just be a number on state instead of three different flags. Then in your buttons, you can use this.state.donation === 50 in your ternary expressions.

For that matter, I probably would have made those buttons a subcomponent to avoid repeating myself.

Yeah, and in Form, all those inputs, I probably would have made those subcomponents.

Oh well, just some thoughts. All in all it looks good. Keep in mind that I’m a picky reviewer.

3

Thank you very much, I’ve applied your suggestions. Some of them are so mindblowingly simple and I see that I’ve some things overcomplicated. Again, thanks :slight_smile:

1 Like