7
\$\begingroup\$

I'm learning React and I request you to review it.

Preview it here

import React, {Component} from 'react';
import './Country.css';

class Country extends Component {
    constructor(props) {
        super(props);
        this.state = {
            countries: [],
            randomCountry: {},
            randomOptions: [],
            userIsWin: '',
            disableFieldset: false,
            goodGuess: 0,
            bgColor: {backgroundColor: 'white'}
        }
        this.getRandomCountry = this.getRandomCountry.bind(this);
        this.checkWin = this.checkWin.bind(this);
    }

componentDidMount() {
    const apiUrl = "https://restcountries.eu/rest/v2/all";
    fetch(apiUrl)
    .then(data => data.json())
    .then(countries => this.setState({countries}))
    .then(this.getRandomCountry)
}


getRandomCountry() {
    const random = this.state.countries[Math.floor(Math.random()*this.state.countries.length)];
    const randomOpt1 = this.state.countries[Math.floor(Math.random()*this.state.countries.length)];
    const randomOpt2 = this.state.countries[Math.floor(Math.random()*this.state.countries.length)];
    const randomOpt3 = this.state.countries[Math.floor(Math.random()*this.state.countries.length)];
    const randomOptions = [random.name, randomOpt1.name, randomOpt2.name, randomOpt3.name];
    randomOptions.sort(() => { return 0.5 - Math.random() });
    this.setState({
        randomCountry: random,
        randomOptions: randomOptions,
        userIsWin: '',
        disableFieldset: false
    })
}

checkWin(e) {
    this.setState({
        disableFieldset: true
    })
    const winCountry = this.state.randomCountry.name;
    const userGuess = e.target.value;
    if (winCountry === userGuess) {
        this.setState({
            userIsWin: 'Win',
            goodGuess: this.state.goodGuess + 1,
            bgColor: {backgroundColor: '#81C784'}
        })
    } else {
        this.setState({
            userIsWin: 'Lose',
            bgColor: {backgroundColor: '#FF8A65'}
        })
    }
    setTimeout(()=>{
        this.getRandomCountry();
        this.setState({
            userIsWin: '',
            disableFieldset: false,
            bgColor: {backgroundColor: 'white'}
        })
        console.log(e.target)
    }, 2000)

}


render() {
    return (
        <div className="main" style={this.state.bgColor}>
            <div className="wrapper">
                <h1>Country Guessing Game</h1>
                <button className="rnd mui-btn mui-btn--raised" onClick={this.getRandomCountry}>Random</button>
                <div className="img-container">
                    <img className="mui-panel" src={this.state.randomCountry.flag} alt="Country flag" />
                </div>
                <h2>{this.state.userIsWin === 'Win' ? 'You guess right! ' : ''}
                    {this.state.userIsWin === 'Lose' ? 'You guess wrong. ' : ''} 
                    Score:{this.state.goodGuess}</h2>
            </div>
            <fieldset disabled={this.state.disableFieldset}>
                <form onClick={e => this.checkWin(e)}>
                    <button className="mui-btn mui-btn--raised" value={this.state.randomOptions[0]}>{this.state.randomOptions[0]}</button>
                    <button className="mui-btn mui-btn--raised" value={this.state.randomOptions[1]}>{this.state.randomOptions[1]}</button>
                    <button className="mui-btn mui-btn--raised" value={this.state.randomOptions[2]}>{this.state.randomOptions[2]}</button>
                    <button className="mui-btn mui-btn--raised" value={this.state.randomOptions[3]}>{this.state.randomOptions[3]}</button>
                </form>
            </fieldset>
        </div>
    )
}
}

export default Country;
\$\endgroup\$
  • \$\begingroup\$ When running your preview on my computer, if I click on the bottom of the page, a click event is captured and I get a "you guess wrong". \$\endgroup\$ – doubleOrt Jan 5 '18 at 13:55
  • 1
    \$\begingroup\$ Thanks for feedbuck, you get right. I must correct it \$\endgroup\$ – Franky Jan 6 '18 at 0:35
  • 2
    \$\begingroup\$ Welcome to Code Review, I'd recommend adding a little bit more pure-English description to your post, such as the description of what your code does and why you decided to code this (your link helps A LOT though and is why you are getting so many up-votes). For future reference I'd recommend taking a look at Simon's Guide to posting a good question. \$\endgroup\$ – Simon Forsberg Jan 11 '18 at 22:55
6
\$\begingroup\$

The screen design looks very nice. It is minimalistic, yet pleasing. It almost perfectly fits on the small screen of my phone (only sometimes, I have to scroll down to see all the answer buttons). The flags are nicely big to explore the fine graphical details. To appreciate the shape of the Nepal flag, there should be no border around it, and the background color could be set to something other than white, since that is used in some of the flags.

Bug: never sort using a random comparator. Instead, shuffle the array. This has been answered often already, just search for the above terms.

When reading the code aloud, "a country is a component" doesn't make sense. A country is something having a name and a flag, so the class should better be named CountryComponent.

Most JavaScript code can be rewritten to not use the this keyword or the bind function, which leads to clearer and simpler code. See this code review for an example.

In the setTimeout call, I prefer to extract the first argument into a named function, so that the setTimeout appears closer to the 2000:

function action() {}

setTimeout(action, 2000);

There should be a space between the label "Score:" and the score number.

When I play the quiz, my total score always stays at 0, so there must be something wrong. After each click on one of the answer buttons, the page becomes green or orange for about a second and is then reloaded, which explains that the score is reset and that the screen flickers. On the other hand, the Random button works fine, so there must be something that differs between those. (Tested with Firefox on Android.)

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thanks for feedback, i consider shuffle method, but you got me I go easy way there ;( I agree with component name, thanks. There should be space, you got right. I correct this things. Total score works great on chrome (i develop in this browser), and score dont reset. I dont know why this is not working on firefox (im in react since 2 days). There is something like prefixers for css in react? I thought that babel care about everything \$\endgroup\$ – Franky Jan 6 '18 at 0:33
6
\$\begingroup\$

Feedback

Nice use of arrow functions, the fetch API (and associated promises), etc. The layout looks nice as well.

Suggestions/Review Points

Options could appear multiple times

The code in getRandomCountry() does not account for the scenario where Math.floor(Math.random()*this.state.countries.length) yields the same value within 4 consecutive calls. Even with 250 options, it is possible to have one country name appear 2+ times in the list of options. Instead of selecting three random options, a loop could be used until 4 unique names are selected.

So instead of these lines:

const random = this.state.countries[Math.floor(Math.random()*this.state.countries.length)];
const randomOpt1 = this.state.countries[Math.floor(Math.random()*this.state.countries.length)];
const randomOpt2 = this.state.countries[Math.floor(Math.random()*this.state.countries.length)];
const randomOpt3 = this.state.countries[Math.floor(Math.random()*this.state.countries.length)];
const randomOptions = [random.name, randomOpt1.name, randomOpt2.name, randomOpt3.name];

Use a while (or a for) loop to continue adding options. Inside the loop, select the "random" country name and then Array.includes() can be used to check if each random name does (not) exist already in the array:

const random = this.state.countries[Math.floor(Math.random()*this.state.countries.length)];
let randomOptions = [random.name];
while(randomOptions.length < 4 ){ 
    const randomOpt = this.state.countries[Math.floor(Math.random()*this.state.countries.length)];
    if (!randomOptions.includes(randomOpt.name)) { 
        randomOptions.push(randomOpt.name);
    }
}

Inconsistent color specifications

While there is nothing wrong with it, there is an inconsistency with the specification of colors (e.g. white for the initial background color, then either #81C784 for a win or #FFA865 for a loss). Others reading your code might be thrown off by the difference in specification. At minimum, a helpful comment after the hex codes could be helpful... For example:

bgColor: {backgroundColor: '#81C784'} //green

and

bgColor: {backgroundColor: '#FFa865'} //orange
|improve this answer|||||
\$\endgroup\$
  • 4
    \$\begingroup\$ Down-voters: would you mind explaining what could be improved here? If you care enough to lose a reputation point, why not help the community by allowing us to improve??? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jan 5 '18 at 16:50
  • 1
    \$\begingroup\$ Hey, thanks for feedback, every is helpful while learning. I'm really surprised that its well made because im learning since 2 days. About backgorund color as white - I just forget hex code for white and put 'white' :D i didnt know this i a problem, i will change this. \$\endgroup\$ – Franky Jan 6 '18 at 0:26
  • \$\begingroup\$ This answer is being discussed on Meta. \$\endgroup\$ – 200_success Jan 11 '18 at 22:17
2
\$\begingroup\$

There are a few things that I think are common practices in early 2018:

First, call super with ...args, like in

class Country extends Component {
  constructor(...args) {
    super(...args);
    // all the things
  }
}

Next, while I'm personally good with 4 occurrences of the same operation in this particular example, but if you detect a pattern and have a strong sensation that it will repeat in future, it's a good idea to spin it off into a separate pure function. It's usually a fight between programmer's desire to show off and that same programmer's understanding that the code has to be balanced on maintainability-universality scale.

// OKAY
const random = this.state.countries[Math.floor(Math.random()*this.state.countries.length)];
const randomOpt1 = this.state.countries[Math.floor(Math.random()*this.state.countries.length)];
const randomOpt2 = this.state.countries[Math.floor(Math.random()*this.state.countries.length)];
const randomOpt3 = this.state.countries[Math.floor(Math.random()*this.state.countries.length)];

// BETTER?
function randomItem(items = []) {
  return items[Math.floor(Math.random() * items.length)];
}

const random = randomItem(this.state.countries);
const randomOpt1 = randomItem(this.state.countries);
const randomOpt2 = randomItem(this.state.countries);
const randomOpt3 = randomItem(this.state.countries);

// BETTER? Although it's getting obscure, beware!
const randomOptions = Array(4).fill().map(() => randomItem(this.state.countries).name)

Sorting that comes immediately after is unnecessary. Also, take note that Array#sort is a non-stable operation, meaning it returns a sorted array while also mutating the original array. In this particular case, it's fine, but in some cases that may cause unexpected behavior.

Math.random was used to pick items from this.state.countries, so it's simply irrational to sort the array. The values are already random, it doesn't add value to shuffle the array one more time.

It's recommended to pass a function to setState (see docs), so every call to this.setState could accept a function that returns an object. It's relevant when it's possible that your component may in some circumstances execute more than one setState at once.

// OLD
this.setState({
  goodGuess: this.state.goodGuess + 1,
});

// NEW
this.setState(prevState => ({
  goodGuess: prevState.goodGuess + 1,
});

The userIsWin property of state, in my opinion, should be boolean; it's also a common practice to name boolean variables, properties and predicates beginning with "is". But it may mean 1) user didn't begin the game, 2) in current round, user won, 3) in current round, user lost. How to deal with its ternary nature then? I'd suggest using undefined as a value to support the first case:

class Country extends Component {
  constructor(...args) {
    super(...args);
    this.state = {
      isWin: undefined,
    }
  }

  checkWin(e) {
    // do all the things
    this.setState(() => {
      isWin: winCountry === userGuess,
    });

    setTimeout(() => this.setState(() => ({
      isWin: undefined,
    })), 2000);
  }
}

The red and green background are controlled by component state. They shouldn't. The background color is simply reflection of whether the user has won. You can even notice a pattern: background color is updated if and only if userIsWin property is updated. It should go to render method. More than that, because you refer to CSS class name, it actually should go to CSS.

render() {
  return (
    <div className=`main ${this.state.isWin === true ? 'main-green' : this.state.isWin === false ? 'main-red' : ''}`>
      all the things
    </div>
  );
}

If you use ESLint, there's a chance it'll throw an error because "no-nested-ternary" rule is violated. It's up to you how to make ESLint happy, but my first suggestion would be to split the condition:

render() {
  return (
    <div className=`main ${this.state.isWin === true && 'main-green'} ${this.state.isWin === false && 'main-red'}`>
      all the things
    </div>
  );
}

And the last thing. Current implementation relies on Event in checkWin method. A common practice in React is to instead mention the value explicitly in onClick prop:

// OKAY
<button className="mui-btn mui-btn--raised" value={this.state.randomOptions[0]}>{this.state.randomOptions[0]}</button>
<button className="mui-btn mui-btn--raised" value={this.state.randomOptions[1]}>{this.state.randomOptions[1]}</button>
<button className="mui-btn mui-btn--raised" value={this.state.randomOptions[2]}>{this.state.randomOptions[2]}</button>
<button className="mui-btn mui-btn--raised" value={this.state.randomOptions[3]}>{this.state.randomOptions[3]}</button>

// BETTER
<button
  className="mui-btn mui-btn--raised"
  value={this.state.randomOptions[0]}
  onClick={() => this.checkWin(this.state.randomOptions[0])}
>
  {this.state.randomOptions[0]}
</button>
<button
  className="mui-btn mui-btn--raised"
  value={this.state.randomOptions[1]}
  onClick={() => this.checkWin(this.state.randomOptions[1])}
>
  {this.state.randomOptions[1]}
</button>
<button
  className="mui-btn mui-btn--raised"
  value={this.state.randomOptions[2]}
  onClick={() => this.checkWin(this.state.randomOptions[2])}
>
  {this.state.randomOptions[2]}
</button>
<button
  className="mui-btn mui-btn--raised"
  value={this.state.randomOptions[3]}
  onClick={() => this.checkWin(this.state.randomOptions[3])}
>
  {this.state.randomOptions[3]}
</button>

// EVEN BETTER
{this.state.randomOptions.map(name => (
  <button
    key={name}
    className="mui-btn mui-btn--raised"
    value={name}
    onClick={() => this.checkWin(name)}
  >
    {name}
  </button>
))}

That way, the component is truly a reflection of state into DOM and event handlers are reflections of state into state, which is a clean React practice.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Wow, thanks for amazing feedback. About (...args) its cosmetic only? About this randoms, I didn't get it at first (i must study your example calmly - still fresh guy in js/react), Your right about setState with prevState, i will correct this. Wow I get problem with isWin state, i tried a false/true but this don't work so switch to string, but I don't knew that it can be done with undefined (I must try this). Thanks about background color pattern. About onClick methon in buttons, you mean to get rid of <form> element and insted use onClick to all buttons? \$\endgroup\$ – Franky Jan 6 '18 at 0:49
0
\$\begingroup\$

Both the code and the game look very good! Just a few little thoughts:

I don't think you need:

this.getRandomCountry = this.getRandomCountry.bind(this);
this.checkWin = this.checkWin.bind(this);

In a class you can call your own methods without binding them. If that gives you an error you're probably losing scope somewhere else

class Thing {
   constructor(){
      this.doThing()
   }
   doThing(){
   }
}

You could make 'win' a boolean and use it for both right and wrong:

<h2>{(this.state.userIsWin) ? 'You guess right! ' : 'You guess wrong.'}</h2>
|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ It is necessary to bind a class method to the instance, because it gets passed to another component; without being bound to current context, it will throw an error. This is a very typical React thing. Please correct me if I'm wrong. \$\endgroup\$ – rishat Jan 5 '18 at 18:27
  • \$\begingroup\$ Hey, thanks for feedback. I don't know much about this .bind(this) method (im learn react since 2 days), but its recommended in react official site docs, so something must be in. I probably get errors in react developer site when they are dont here. And about this last h2 element i use to do it like you, but i return false default so its rendered 'You guess wrong' even before you guess. \$\endgroup\$ – Franky Jan 6 '18 at 0:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for?Browse other questions tagged or ask your own question.