Skip to content

code cleanup#18

Open
lishamohan wants to merge 7 commits intomasterfrom
cleanup-main
Open

code cleanup#18
lishamohan wants to merge 7 commits intomasterfrom
cleanup-main

Conversation

@lishamohan
Copy link
Contributor

No description provided.

@reginawang3495 reginawang3495 self-requested a review October 14, 2021 05:03
Comment on lines +3 to +11
// check that all zebra colors are different
const checkZebras = (zebraColors) => (new Set(zebraColors)).size === zebraColors.length;

// check that all leopard colors are the same as the background
const checkLeopards = (leopardColors, backgroundColor) => leopardColors.every((e) => e === backgroundColor)

// check that all plant colors are rgb(48, 98, 48)
const checkPlants = (plantColors) => plantColors.every((e) => e === "rgb(48, 98, 48)")

Copy link
Contributor

Choose a reason for hiding this comment

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

are all zebras supposed to be different colors always? so are all animals supposed to be a certain way regardless of level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes! and all the levels should be designed with those rules in mind

Copy link
Contributor

@timothycpoon timothycpoon left a comment

Choose a reason for hiding this comment

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

Mostly seems good, just a couple comments

@@ -1,17928 +1,8 @@
{
"name": "teach-la-react-starter-barebones",
"version": "0.1.0",
"lockfileVersion": 2,
Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably stick to newer versions of NPM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay i updated it


const check = (boardEl, setStickerStyles, backgroundColor) => {
let stickerStyles = {};
let zebraColors = []; //for plant colors test to work an element will need to be added to this array
Copy link
Contributor

Choose a reason for hiding this comment

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

Depending on how much you foresee extending this, consider having a map of arrays instead of adding an array for each new entity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed!

@lishamohan lishamohan marked this pull request as ready for review October 15, 2021 18:37
@lishamohan
Copy link
Contributor Author

I added a checks object to constants.js with the checks so they aren't hardcoded. I think it's kinda hacky though so lmk if you guys think there's a better way


const check = (boardEl, setStickerStyles, backgroundColor) => {
let stickerStyles = {};
let colorsToCheck = checks.map(() => []);
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry if I was unclear; when I mentioned map I meant like

{
zebra: [],
leopard: [],
zebra: []
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i see i just thought this would be better. should i change it back and just use that?

Copy link
Contributor

Choose a reason for hiding this comment

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

This method works fine, it just might be a bit more confusing when you're debugging the state, and just see an array of arrays.

}
});
stickerStyles[curNode.id] = stickerColor;
setStickerStyles(stickerStyles);
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't need to update the state on every iteration

{
type: "tagName",
value: "LEOPARD",
check: (leopardColors, backgroundColor) => leopardColors.every((e) => e === backgroundColor)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of passing in backgroundColor as the second input, some sort of options object would be better, so that the number of arguments wouldn't keep rising as new animals are added.


const check = (boardEl, setStickerStyles, backgroundColor) => {
let stickerStyles = {};
let colorsToCheck = checks.map(() => []);
Copy link
Contributor

Choose a reason for hiding this comment

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

This method works fine, it just might be a bit more confusing when you're debugging the state, and just see an array of arrays.

return colorsToCheck.every((colors, i) => checks[i].check(colors, backgroundColor));
}

export default check; No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

Having a newline at the end of files is a good convention to keep

value: "plant",
check: (plantColors, _) => plantColors.every((e) => e === "rgb(48, 98, 48)")
}
]; No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing w/ newline

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