-
Notifications
You must be signed in to change notification settings - Fork 15
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
Module #3 (Tahsina Bintay Azam) #35
base: main
Are you sure you want to change the base?
Conversation
src/hooks/snake.js
Outdated
@@ -115,9 +115,9 @@ const UseSnake = () => { | |||
}, [direction, isFood, isSnake, resetGame]); | |||
|
|||
useInterval(runSingleStep, 200); | |||
useInterval(addFood, 3000); | |||
useInterval(addObject("food"), 3000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useInterval(()=>addObject("food"), 3000);
src/hooks/snake.js
Outdated
useInterval(removeFood, 100); | ||
useInterval(addPoison, 15000); | ||
useInterval(addObject("poison"), 15000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useInterval(()=>addObject("poison"), 3000);
@MahdiMurshed thanks a lot, I did what you suggested and my code works just fine <3 |
src/hooks/snake.js
Outdated
setObjects([]); | ||
}, []); | ||
|
||
const isFood = useCallback( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isFood and isPoison are basicallly same.Can't we make one function to check both ? like
isObjectOfType(type)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes added that :) thanks for pointing it out <3
src/hooks/snake.js
Outdated
//suggested addObject method for food and poison ----------------> | ||
const addObject = useCallback((type) => { | ||
let newObject = getRandomCell(type); | ||
while (isSnake(newObject) || isFood(newObject)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you missed isPoison()
src/hooks/snake.js
Outdated
// } | ||
}, []); | ||
//removeObject method for removing food or poison--------------> | ||
const removeObject = useCallback((type) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are deleting objects whose age are greater than 10 sec .So why do we need to check object type while deleting any object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, you were right, for some reason that part of the code was causing the problem, I thought I should have the control over when I wanted to remove my poison cell that's why I was checking object Type. Thanks btw.
src/hooks/snake.js
Outdated
setFoods((currentFoods) => | ||
currentFoods.filter((food) => Date.now() - food.createdAt <= 10 * 1000) | ||
setObjects((currentFoods) => | ||
currentFoods.filter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're removing an element whether it is food or poison for the same time-lapse. We don't need to check the type here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I totally agree. For some reason that part of the code was causing the problem, I thought I should have the control over when I wanted to remove my poison cell that's why I was checking object Type. Thanks btw.
src/hooks/snake.js
Outdated
setObjects((currentFoods) => | ||
currentFoods.filter( | ||
(food) => | ||
Date.now() - food.createdAt <= 10 * 1000 && food.type === "food" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use the CellType here instead of the string.
food.type===CellType.Food
src/hooks/snake.js
Outdated
useInterval(() => addObject("food"), 3000); | ||
useInterval(() => removeObject("food"), 100); | ||
useInterval(() => addObject("poison"), 15000); | ||
useInterval(() => removeObject("poison"), 100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we dont need to call removeObject twice and removeObject doesn't need to take any parameter
src/hooks/snake.js
Outdated
useInterval(runSingleStep, 200); | ||
useInterval(() => addObject("food"), 3000); | ||
useInterval(() => removeObject("food"), 100); | ||
useInterval(() => addObject("poison"), 15000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are not taking the advantage of CellType object .By hard coding "food" you are creating a source of bug . Do It instead
addObject(CellType.Food)
removeObject(CellType.Poison)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@owishiboo and @MahdiMurshed used the cell type 👍
src/hooks/snake.js
Outdated
useInterval(() => addObject(CellType.Food), 3000); | ||
useInterval(() => removeObject(CellType.Food), 100); | ||
useInterval(() => addObject(CellType.Poison), 15000); | ||
useInterval(() => removeObject(CellType.Poison), 100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry,removeFood doesn't need any parameter, my bad.And why calling twice the same function at same time interval? you are not removing the object depending on types .You are removing objects whose age is greater than 10 sec regardless of the type.
useInterval(() => addObject(CellType.Food), 3000);
useInterval(() => addObject(CellType.Poison), 15000);
useInterval(removeObject, 100);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dang , missed that part :")
isSnake(newObject) || | ||
isObjectOfType(newObject, CellType.Food) || | ||
isObjectOfType(newObject, CellType.Poison) | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as you previously forgot to add isPoison() in while condition, There is a possibility that you will forget it again. So why not create a separate function named isObject whose job is to check whether the given coordinate is a food or poison .
tried refactoring and tried to merge food and poison into the same state.