Skip to content

First Commit. Update readme. Flowchart in img folder#21

Open
mingyikoh wants to merge 30 commits into
wdi-sg:masterfrom
mingyikoh:master
Open

First Commit. Update readme. Flowchart in img folder#21
mingyikoh wants to merge 30 commits into
wdi-sg:masterfrom
mingyikoh:master

Conversation

@mingyikoh
Copy link
Copy Markdown

Readme is updated.
Flowchart has two versions, one has better formatting. Delete other one in next commit.
Start work on coding game logic.

Comment thread assets/js/script.js Outdated
var counter = 1

// constructor for all spawns
function Spawn(life, damageWhenExpire, effectHealth, effectAmmo, effectGrenade, timeToExpire, counterLink){
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are we not using a class method here?

Comment thread assets/js/script.js
$spawn.attr('id', "s"+counter)
$spawn.addClass('spawn enemyEasy')
$spawn.css({
"left": Math.floor(Math.random() * 1000),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why don't you randomize the position in the constructor instead?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

My intention was to separate the functions that are HTML/CSS from the JS side of things.

For my constructor I only wanted to include 'game effect stats' that had nothing to do with the HTML and CSS side of things. This is mainly because I was hoping to add more variety to enemy stats in the future, for example adding scores, or making boss enemies that have more life and stay for longer but deal set damage every few seconds. I was worried adding CSS inside the constructor would make it look too messy.

For the setting of ID, Class, HTML and CSS side of things, I was hoping to chunk them in a separate function. I realise that it resulted in a lot of repetition for my spawnEnemyEasy(), spawnEnemyNormal() and all the rest. My solution would be to create a separate function named spawnDiv() which would group the repetitions and replace the repetitions with this function. This would allow the elimination of repetition and still keep the constructor clean imo.

@primaulia
Copy link
Copy Markdown
Contributor

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

Glow

  • Good delivery, nice attempt to invite your classmates to play the game
  • Very fun game, I think you put a good thought into it in terms of design and interaction
  • Good flowchart, it helps me to understand your code flow better

Grow

  • Without flowchart, your code is actually quite hard to follow, try to separate the code in a more logical group next time. Show your codes to your friend, ask them whether they can follow through
  • There are various repetitions in your code that can be refactored

Things to look out for

  • Function flow
  • Separation of concern between design / dom manipulation and game play update

Comment thread assets/js/script.js
@@ -0,0 +1,471 @@
$(function() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If possible, declare variables first at the top of your scope.

Comment thread assets/js/script.js Outdated
$gameOverlay.css({
'display': 'none'
})
document.getElementsByClassName('gogo')[0].volume = 0.6;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You may replace this line $('.gogo.')[0].volume = 0.6. Why does that works but $('.gogo').volume doesn't work?

Comment thread assets/js/script.js Outdated
countDown()
}

$('.easyStartBtn').on('click', () => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You may move your eventListeners after the declarations of functions.

Comment thread assets/js/script.js Outdated

function updatePlayerStats () {
playerStatsInterval = setInterval(() => {
var $playerHealth = $('.playerHealth')
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What will be the difference if you put these line(100 - 103) in outer scope of function updatePlayerStats? What will be the advantage if we reduce the number of DOM accessing?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ah I see, putting the variables in the set interval scope will make unnecessary DOM accessing over and over again.

Comment thread assets/js/script.js
checkEnemyExpire()
checkVictory()
checkLoss()
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

}, 1000)

Comment thread assets/js/script.js
if(spawnList[i].timeToExpire <0){
if(spawnList[i].damageWhenExpire > 0){
playerStats.health = playerStats.health - spawnList[i].damageWhenExpire
document.getElementsByClassName('playerDamage')[0].volume = 0.8
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

How can we reduce DOM accessing?

Comment thread assets/js/script.js
for (var i = spawnList.length -1; i > -1; i --) {
if(spawnList[i].life <= 0) {
$('div').remove('#'+spawnList[i].counterLink)
if (spawnList[i].effectHealth === -1) playerStats.health = playerStats.health -1
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you can do this playerStats.health -= 1

Comment thread assets/js/script.js Outdated
// EVENTS ---------------------------------------------

// 3 easy, ammo refill
function event1(time) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You may use better names for functions. :)

@alexkimin
Copy link
Copy Markdown

alexkimin commented Oct 8, 2017

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

Glow

Very fun game with sound and well-balanced modes.
Good flowchart.

Grow

Naming convention on some functions can be improved.
Reducing DOM accessing if possible.

Things to look out for

ES6 Class
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes

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