trjhtr

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;

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.)

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

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.

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>