Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Project 1 #23

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open

Project 1 #23

wants to merge 25 commits into from

Conversation

CQDOTCOM
Copy link

No description provided.



function shuffleArray(arr) {
for (var i = arr.length - 1; i > 0; i--){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explain this line of codes to me, why does the loop reversing? and how does the es6 code works?

Use simple example.

@@ -0,0 +1,291 @@
//Part 1 - Shuffle array, add a random number btw 1-4 into array(for adding rounds)
var instructorMoves = [37,38,39,40]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid creating a global variables, you can put this under line 21.

return (value - 36);
})

for(let e = 0; e <= randSequence.length; e++){
Copy link
Contributor

@primaulia primaulia Oct 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm? why not just tagged your instructor number class into one div, so you don't have to do this loop instead.

e.g.

var danceMove = instructorMoves [Math.floor( Math.random() * instructorMoves.length )]
$('.instructor').addClass(`dance${danceMove}`)

this.play();
}, false);

$button.on('click' , function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what button is this? Be more descriptive on your variable naming.

$(".startBtn").hide()
$(".resetBtn").show()

$body.on('keydown', (e) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation issues here

// Set keys with different images for player 1
if(strOfKeys === 37) {
$('.playerOneDiv').addClass("move");
$('.playerOneDiv').css('backgroundImage', `url("./assets/images/player1_1.png")`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again same issues here like the instructor moves, why just add in the class instead?

@primaulia
Copy link
Contributor

Project Workflow: 2 / 5
Technical Requirements: 3 / 5
Creativity: 4 / 5
Code Quality: 2 / 5
Problem Solving: 3 / 5
Delivery: 2 / 5
Professional Skill: 3 / 5

Glow

  • Interesting fun game :)
  • Nice HTML layout

Grow

  • Your presentation is dry, please rehearse next time
  • Your code is full of repetitions and many bad formatting, we'll go through each one on the individual consultation
  • The game layout proportion is rather too small at some points
  • Workflow is not clear, it's okay to explain how things work in details, but workflow shouldn't be too detail. It's suppose to help people to understand better, not to confuse them even more

Things to look out for

  • Simplifying your code explanation
  • Proportion on CSS
  • Code refactoring
  • Presentation

@alexkimin
Copy link

Project Workflow: 3 / 5
Technical Requirements: 3 / 5
Creativity: 4 / 5
Code Quality: 2 / 5
Problem Solving: 3 / 5

Glow

Interesting and fun game. :)

Grow

Keep DRY(Don't Repeat Yourself) and KISS(Keep It Simple Stupid) principle in your mind, save your lines and make the logic clear if possible.

Indentation is very important for readability and debugging.
Put more comments on your code for readability.

Avoid using of global scope.

Things to look out for

Code refactoring with DRY principle.

whichPlayer = 2
}

//Part 3 - Matching keypressed with instructorMoves in sequence

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may refactor matching logics for reducing repetition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants