Bughunt round 1 - Githubissues

laukstein commented 2 years ago

First line should be HTML5 Doctype <!doctype html> https://github.com/markodeve/RockPaperScissors/blob/79d8d7eb7dd0eb63659e10d8f922eed1f739b232/index.html#L1

No need for this line because IE is depreceted https://github.com/markodeve/RockPaperScissors/blob/79d8d7eb7dd0eb63659e10d8f922eed1f739b232/index.html#L4

For best code style decide if you use dash - or _ in CSS class names and use it consistently, I recommend using - e.g. top-cont in https://github.com/markodeve/RockPaperScissors/blob/79d8d7eb7dd0eb63659e10d8f922eed1f739b232/index.html#L12

For JS file I recommend you to install npm elsint and run eslint script.js to see recommendations (code analytics issues) for https://github.com/markodeve/RockPaperScissors/blob/main/script.js After all those issues solved, without promises I can go over to see what else I can suggest.

markodeve commented 2 years ago

Hello Binyamin, thank you for the great feedback, the html issues are fixed.

laukstein commented 2 years ago

Great. Are you sure you followed all ESLint errors, it should ask you to change == to === e.g. in https://github.com/markodeve/RockPaperScissors/blob/026316e657adc4f00b73a9a9ef1bcf9ce7830777/script.js#L87

it should show you error missing space before ( in https://github.com/markodeve/RockPaperScissors/blob/026316e657adc4f00b73a9a9ef1bcf9ce7830777/script.js#L144

incorrect code (space) alignment e.g. in https://github.com/markodeve/RockPaperScissors/blob/026316e657adc4f00b73a9a9ef1bcf9ce7830777/script.js#L144

If you use array functions () => {} e.g in https://github.com/markodeve/RockPaperScissors/blob/026316e657adc4f00b73a9a9ef1bcf9ce7830777/script.js#L104 then be consistent and use same syntax also in other places e.g. https://github.com/markodeve/RockPaperScissors/blob/026316e657adc4f00b73a9a9ef1bcf9ce7830777/script.js#L64

Use multiple classNames in Element.classList prototype, e.g. instead of https://github.com/markodeve/RockPaperScissors/blob/main/script.js#L175-L178

            pcIcon.classList.remove('winning-color');
            userIcon.classList.remove('winning-color');
            pcIcon.classList.remove('draw-color');
            userIcon.classList.remove('draw-color');

write

            pcIcon.classList.remove('winning-color', 'draw-color');
            userIcon.classList.remove('winning-color', 'draw-color');

or even write it shorter with Array loop like

[pcIcon, userIcon].forEach(icons => icons.remove('winning-color', 'draw-color'));

Should remove https://github.com/markodeve/RockPaperScissors/blob/main/script.js#L183-L184

See if you can avoid duplicated code or code logic moving it out to separated function and then reuse it e.g. https://github.com/markodeve/RockPaperScissors/blob/main/script.js#L88-L92 https://github.com/markodeve/RockPaperScissors/blob/main/script.js#L95-L99 https://github.com/markodeve/RockPaperScissors/blob/main/script.js#L102-L106 https://github.com/markodeve/RockPaperScissors/blob/main/script.js#L109-L113 https://github.com/markodeve/RockPaperScissors/blob/main/script.js#L116-L120 https://github.com/markodeve/RockPaperScissors/blob/main/script.js#L123-L127 you can pass arguments like icon for the function making it generic.

https://github.com/markodeve/RockPaperScissors/blob/main/script.js#L95-L99

            if (e.target.classList.contains('rock')) {
                playerGuess = 'rock';
            } 
            if (e.target.classList.contains('paper')) {
                playerGuess = 'paper';
            } 
            if (e.target.classList.contains('scissors')) {
                playerGuess = 'scissors';
            }

can be written shorter like

playerGuess = ['rock', 'paper', 'scissors'].find(selected => e.target.classList.contains(selected));

or instead of adding className you could set data attribute like Element.dataset.guess = 'paper'; and then get the value using playerGuess = Element.dataset.guess;.

markodeve / RockPaperScissors

Bughunt round 1 #1