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

Module #3 (Tahsina Bintay Azam) #35

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

tahsina-azam
Copy link

@tahsina-azam tahsina-azam commented Apr 10, 2022

tried refactoring and tried to merge food and poison into the same state.

@@ -115,9 +115,9 @@ const UseSnake = () => {
}, [direction, isFood, isSnake, resetGame]);

useInterval(runSingleStep, 200);
useInterval(addFood, 3000);
useInterval(addObject("food"), 3000);

Choose a reason for hiding this comment

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

useInterval(()=>addObject("food"), 3000);

useInterval(removeFood, 100);
useInterval(addPoison, 15000);
useInterval(addObject("poison"), 15000);
Copy link

@MahdiMurshed MahdiMurshed Apr 12, 2022

Choose a reason for hiding this comment

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

useInterval(()=>addObject("poison"), 3000);

@tahsina-azam
Copy link
Author

@MahdiMurshed thanks a lot, I did what you suggested and my code works just fine <3

setObjects([]);
}, []);

const isFood = useCallback(

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)

Copy link
Author

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

//suggested addObject method for food and poison ---------------->
const addObject = useCallback((type) => {
let newObject = getRandomCell(type);
while (isSnake(newObject) || isFood(newObject)) {

Choose a reason for hiding this comment

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

you missed isPoison()

// }
}, []);
//removeObject method for removing food or poison-------------->
const removeObject = useCallback((type) => {

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?

Copy link
Author

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.

setFoods((currentFoods) =>
currentFoods.filter((food) => Date.now() - food.createdAt <= 10 * 1000)
setObjects((currentFoods) =>
currentFoods.filter(

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.

Copy link
Author

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.

setObjects((currentFoods) =>
currentFoods.filter(
(food) =>
Date.now() - food.createdAt <= 10 * 1000 && food.type === "food"

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

useInterval(() => addObject("food"), 3000);
useInterval(() => removeObject("food"), 100);
useInterval(() => addObject("poison"), 15000);
useInterval(() => removeObject("poison"), 100);

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

useInterval(runSingleStep, 200);
useInterval(() => addObject("food"), 3000);
useInterval(() => removeObject("food"), 100);
useInterval(() => addObject("poison"), 15000);

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)

Copy link
Author

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 👍

useInterval(() => addObject(CellType.Food), 3000);
useInterval(() => removeObject(CellType.Food), 100);
useInterval(() => addObject(CellType.Poison), 15000);
useInterval(() => removeObject(CellType.Poison), 100);

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);

Copy link
Author

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)
) {

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 .

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