Jovon code review

gistfile1.txt

Look for `// CHRIS:` comments to get some inline thoughts.

Well, I learned something new. I've never seen "text/babel" as a script type. How are you transpiling your JSX?

I think if I were interviewing you, I'd ask why you didn't use ES6 modules or TypeScript, and something like Webpack to bundle them. It's something you should familiarize yourself with, since every production stack you will work with will be using some equivalent. (TypeScript + VS Code is a great combo, by the way.)

You'll also benefit from using a linter on your JavaScript. It catches bugs, and ensures you format things nicely (which helps when someone's interviewing / reviewing your code!)

So here's what I'd recommend after a quick scan:

- Use Webpack or some bundler, and move various components into their own files
- Use object destructuring to save lots of lines of code!
- Use object destructuring to destructure props in stateless components
- Don't use html id attribute
- When you see yourself doing something like: `foo[0]; foo[1]`, it should be a flag that you should use a loop / map / filter / reduce (see the render of Faces below)
- It's generally easiest especially on small apps like this to keep all of your mutable state in one place (the root React component), then pass functions down into the child components which will mutate the parent state. In more complex apps, you should move state management out into something like Redux.


```js
// Selects a random integer between 0 and the maximum index of the items array
function getRandomInt (min, max) {
    min = Math.ceil(min);
    max = Math.floor(max);
    return Math.floor(Math.random() * (max - min)) + min;
}

// CHRIS: this could be rewritten this way
/*
function Name(props) {
    const {firstName, lastName} = props.value.item || {};
    return(
        <h2 className="well">
            {`${firstName} ${lastName}`}
        </h2>
    );
}
*/
function Name(props) {
    // CHRIS: The idiomatic way to do this is something like:
    // const {firstName, lastName} = props.value.item || {};

    let fName = '';
    let lName = '';
    if (props.value.item){ // CHRIS: Nitpick: there should be a space before this {, something a linter would catch
        fName = props.value.item.firstName;
        lName = props.value.item.lastName;
    }
    return(
        <h2 id="name" className="well">
            {fName + ' ' + lName}
        </h2>
    );
}

class Face extends React.Component{
    constructor(){
        super()
        this.state = {
            disabled: false,
        }
    }
    render() {
        // CHRIS: I'd just do something like this: const value = this.props.value || {};
        let value = [];
        if(this.props.value) {
            value = this.props.value;
        }
        // CHRIS: Your onClick handler should probably refer to an instance method. My rule of thumb is:
        // if your event handlers need a semicolon, move them to an external function/method
        // Also, this seems like a good candiate for a stateless component, rather than a class-based one,
        // and whatever calls this one can pass in an `onClick` handler and manage state there.
        return(
            <button className="face" id={value.index} onClick={() => {this.props.onClick(); this.setState({disabled:true})} } disabled={this.state.disabled}>
                <img src={value.img} />
            </button>
        );
    }
}

// CHRIS: I usually write these kinds of functions like this:
/*
function Next({onClick) {
    // Destructuring your props like this makes it obvious what kind of props your
    // function expects.
    return (
        <button type="button" className="btn btn-primary btn-lg" onClick={onClick}>
            Next
        </button>
    );
}
*/
function Next(props) {
    return(
        <button id="next" type="button" className="btn btn-primary btn-lg" onClick={() => props.onClick()}>
            Next
        </button>
    )
}

// CHRIS: This is another good candidate for stateless componentry
class Faces extends React.Component {
    renderFace(i) {
        // CHRIS: I would probably rewrite this entirely to assume that props.value was set. 
        // It doesn't make sense to call this component without it. Then, this can be greatly simplified
        // to a simple map operation. (See the render comment below.)
        let imgLocation = '';
        if(this.props.value) { 
            if(this.props.value[i] !== undefined){
                imgLocation = this.props.value[i].headshot.url;
            } else {
                imgLocation = ""; // CHRIS: Consistent use of "" vs '' is something to watch for. I prefer ''
            }
        }
        let value = {img: imgLocation, index: i};
        // CHRIS: I find it useful to always use parens when returning JSX
        // return (
        //   <Face ...
        // )
        return <Face value={value} onClick={() => this.props.onClick(i)}/>;
    }
    render() {
        // CHRIS: I'd do something more like the following:
        /*
          // Elsewhere, I'd sanitize my data to ensure there are no invalid values in the array so
          // that these components don't need to worry about that.
          // Note also that Face has been changed to take url and index as separate props, making it easier 
          // to invoke.
          const {value, onClick} = this.props;
          return (
            <div className="faces">
              {value.map((v, i) => (
                <Face url={v.headshot.url} index={i} onClick={onClick} />
              ))}
            </div>
          );
        */
        return (
            <div className="faces">
                {this.renderFace(0)}
                {this.renderFace(1)}
                {this.renderFace(2)}
                {this.renderFace(3)}
                {this.renderFace(4)}
            </div>
        );
    }
}

class Game extends React.Component {
    constructor() {
        super();
        this.state = {
            history: {
                right: 0,
                wrong: 0,
            },
            data: [],
            choices: [],
            maxIndex: 0,
            correctlyAnswered: false,
            answer: {}
        }
        var obj = this;
        this.event = (e) => {
                // CHRIS: I'd comment what these numbers mean. No magic numbers!
                // Also, this is a great candidate for a switch statement.
                // CHRIS: This is as far as I could get. Gotta run to a meeting!
                if(e.keyCode === 49){
                    obj.handleChoiceClick(0);
                } 
                if(e.keyCode === 50) {
                    obj.handleChoiceClick(1);
                }
                if(e.keyCode === 51){
                    obj.handleChoiceClick(2);
                } 
                if(e.keyCode === 52){
                    obj.handleChoiceClick(3);
                } 
                if(e.keyCode === 53){
                    obj.handleChoiceClick()
                } 
                if(e.keyCode === 13){
                    obj.handleNextClick();
                }

            }
        
    }
    componentDidMount(){
        var obj = this;
        fetch("https://willowtreeapps.com/api/v1.0/profiles/")
            .then( (response) => {
                return response.json() })   
                    .then( (json) => {
                        this.setState({data: json.items, maxIndex: json.items.length - 1});
                        this.getChoices();
                    });

            window.addEventListener('keyup', this.event);
    }
    componentWillUnmount(){
        window.removeEventListener('keyup');
    }
    handleChoiceClick(i) {
        let rightNum = this.state.history.right;
        let wrongNum = this.state.history.wrong;
        let correctlyAnswered = this.state.correctlyAnswered;
        if(correctlyAnswered === false) {
            if(i === this.state.answer.index){
                rightNum++;
                correctlyAnswered = true;
            } else {
                wrongNum++;
            }
            this.setState({
                            history: {
                                right: rightNum, 
                                wrong: wrongNum
                            }, 
                            correctlyAnswered: correctlyAnswered
                            }
                        );
        }
    }
    handleNextClick(){
        this.getChoices();
        this.setState({correctlyAnswered: false});
    }
    // Creates an object with the chosen people
    getChoices(){
        let maxChoices = 5;
        let choicesIndexes = this.pickChoices(maxChoices);
        let choices = [];
        for(let i = 0; i < maxChoices; i++){
            choices.push(this.state.data[choicesIndexes[i]]);
        }
        
        let answer = this.pickAnswer(choices);
        this.setState({choices: choices, 
                       answer: {index: answer, 
                                item: choices[answer]}
                       });

    }
    // Picks 5 random integers to be indexes of the items array
    //   which represent the possibles choices.
    pickChoices (maxChoices) {
        let chosenIndexes = []
        let max = this.state.maxIndex;
        while(chosenIndexes.length < maxChoices){
            let indx = this.pickRandomIndex(max)
            if(!this.isChosen(indx, chosenIndexes)){
                chosenIndexes.push(indx);
            }
        }
        return chosenIndexes;
    }

    // Return a random integer with the index in the items array 
    // Arguments:
    //      maxIndex = the highest possible index of the items array
    pickRandomIndex (max) {
        return getRandomInt(0, max);
    }

    // Returns the index of the randonly pick answer
    pickAnswer (choices) {
        return this.pickRandomIndex(choices.length - 1); 
    }


    // Was this index already chosen?
    isChosen (indx, chosenIndexes) {
        return undefined !== chosenIndexes.find(function(v){
            return v === indx;
        })
    }

    render() {
        return (
            <div className="game">
                <div className="question">
                    <h1>Who is?</h1>
                    <Name value={this.state.answer || {}} />
                </div>
                <Faces value={this.state.choices || []} onClick={(i)=> this.handleChoiceClick(i)}/>
                <div className="history">
                    <div id="right">Right: {this.state.history.right}</div>
                    <div id="wrong">Wrong: {this.state.history.wrong}</div>
                 </div>
                 <div className="next">
                     <Next onClick={()=> this.handleNextClick()} />
                 </div>

            </div>
        );

    }
}

ReactDOM.render(<Game />, document.getElementById('container'));