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

niikita's snakes and ladders w/ gameplay n flow #33

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

Conversation

nikitas89
Copy link

No description provided.

<!-- grid elements -->
<div class="wrapper">

<div class="box" data-id="100"></div>
Copy link

Choose a reason for hiding this comment

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

Recommend using a js loop to append these div elements to the .wrapper div. Data id can be added using jquery's .attr(), or .data(), or an id or class can also be used.

Copy link

Choose a reason for hiding this comment

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

append order can be 100-> 91, 81-> 90, 80-> 71 etc.

<div class="box" data-id="9"></div>
<div class="box" data-id="10"></div>
</div>
<div class="left">
Copy link

Choose a reason for hiding this comment

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

aside can also be used.

})

var runGame = () => {
$('div#gametext').show()
Copy link

@soemn soemn Oct 6, 2017

Choose a reason for hiding this comment

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

$('#gametext') is enough

width: 80px;
height: 80px;
transition: all .2s ease-in;
opacity: 0;
Copy link

Choose a reason for hiding this comment

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

is opacity needed? there is no background.

background-repeat: no-repeat;
background-position: center;
background-size: 100%;
opacity: 1
Copy link

Choose a reason for hiding this comment

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

no-repeat not required if size is bigger than div and set to 100%.
Opacity do not need to be set to 1 if it was not set to 0 beforehand.

<h3 id="newPos"></h4>
<h3 id="jump"></h4>
<h1 id="winner"></h1>
</div>
Copy link

@soemn soemn Oct 6, 2017

Choose a reason for hiding this comment

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

indentation for various parts need to be updated.

@@ -0,0 +1,68 @@

Copy link

Choose a reason for hiding this comment

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

indentation style is not consistent

Choose a reason for hiding this comment

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

reset.css is not done by her. It is resetting helper.

// console.log('rollValue', roll);
var nextPos = lastMove + roll
// console.log('jump', getJump(nextPos));
move = getJump(nextPos) === 0 ? nextPos : getJump(nextPos)
Copy link

Choose a reason for hiding this comment

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

When the character move, the grid is updated but the old position is not deleted. Hence if the character takes a snake down. On the next character's turn, it takes the character's furthest traveled position due to grid.lastIndexOf() and starts the next position move from there, this invalidates the role of snakes in the game.

Choose a reason for hiding this comment

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

agree. grid should have only one 1 and 2.

}

function getJump(rollValue) {
if (jumps[rollValue]) {
Copy link

Choose a reason for hiding this comment

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

maybe update to
if key is valid, return value.
instead of if value is valid, return value. Both works the same though


}

var player = 1

Choose a reason for hiding this comment

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

you may put variables to the top.


//function set grid
function playTurn() {
// console.log('PLAYER START:', player);

Choose a reason for hiding this comment

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

If possible, delete console.log() that is commented out. :)

}

function changePlayer() {
player === 1 ? player = 2 : player = 1

Choose a reason for hiding this comment

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

you may try this player = player === 1 ? 2 : 1

player === 1 ? player = 2 : player = 1
}

$(document).ready(function() {
Copy link

@alexkimin alexkimin Oct 10, 2017

Choose a reason for hiding this comment

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

I recommend you wrap all variables and functions into the ready function. $(function() { .... })
It will help you to avoid putting your variables and functions into global scope.
Everything in the global scope is weak to manipulation by others.


$(document).ready(function() {
var $dice = $("#dice")
$dice.on('click', runGame)

Choose a reason for hiding this comment

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

What happens if I put runGame() as a second argument? And why?


var runGame = () => {
$('div#gametext').show()
if (whoWon()) {

Choose a reason for hiding this comment

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

You may use ternary operator here.

var $jump = $("#jump")

function showTexts(lastMove, roll, newPos, move, player) {
$diceImg.attr('src', "./assets/images/Dice-" + roll + ".png")
Copy link

@alexkimin alexkimin Oct 10, 2017

Choose a reason for hiding this comment

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

move = getJump(nextPos) === 0 ? nextPos : getJump(nextPos)
// console.log('move', move)
showTexts(lastMove, roll, nextPos, move, player)
move > 100 ? move = 100 : grid[move] = player
Copy link

@alexkimin alexkimin Oct 10, 2017

Choose a reason for hiding this comment

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

You may do this with if statement.

If(move > 100) move = 100
grid[move] = player

// console.log('rollValue', roll);
var nextPos = lastMove + roll
// console.log('jump', getJump(nextPos));
move = getJump(nextPos) === 0 ? nextPos : getJump(nextPos)

Choose a reason for hiding this comment

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

agree. grid should have only one 1 and 2.

@@ -0,0 +1,91 @@
var grid = Array(100).fill(0)
Copy link

@alexkimin alexkimin Oct 15, 2017

Choose a reason for hiding this comment

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

I gave you inappropriate answer about this, so remain new answer here.

Do you remember your var array = Array(10).fill(Array(10).fill(0)) was acting weird?

var array = Array(10).fill(Array(10).fill(0))

var shallowCopied = array.slice()
var deepCopied = array.map(e => e.slice())

array[0][3] = 1

console.log(shallowCopied)
console.log(deepCopied)

Try above.

That was because .slice() is just shallow copy, inside arrays of 2D were not copied at all. So, when we re-assign value there, our original array was changed also due to sharing references.

I hope you google about shallow copy vs deep copy if you want to know more. :)

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