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.

Sign up for GitHub

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

Jump to bottom

Clean up new Maze Class #20411

Merged
merged 5 commits into from Feb 9, 2018
Merged

Clean up new Maze Class #20411

merged 5 commits into from Feb 9, 2018

Conversation

Hamms
Copy link
Contributor

@Hamms Hamms commented Feb 3, 2018 •
edited

Follow-up to #20408

  • make as many of Maze's instance methods pseudo-private (by prefixing with underscores) as possible, to help identify exactly what kind of contract Maze is presenting to the rest of the codebase.
  • declare all Maze instance properties in the constructor, to help identify exactly what kind of contract Maze is presenting to itself

@Hamms Hamms force-pushed the maze-to-class branch 2 times, most recently from 45c8c14 to 7ee525c Compare February 5, 2018 22:52
@Hamms Hamms force-pushed the maze-to-class branch 2 times, most recently from 78d3fab to ad88f59 Compare February 6, 2018 19:55
@Hamms Hamms changed the title [WIP] Clean up Maze Class Clean up new Maze Class Feb 6, 2018
balderdash
balderdash approved these changes Feb 7, 2018
View reviewed changes
apps/src/maze/maze.js
this.pegmanX;
this.pegmanY;

this.MAZE_HEIGHT;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since these aren't actually constants, we may want to stop capitalizing them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed; the goal of this PR is mostly to help draw the limits of what can and cannot be easily refactored, I'm gonna do most of the actual refactoring in other PRs

apps/src/maze/maze.js Outdated
@@ -124,10 +153,10 @@ module.exports = class Maze {
this.PATH_WIDTH = this.SQUARE_SIZE / 3;
}

createAnimations(svg) {
_createAnimations(svg) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I've seen a lot more "private" methods in our codebase with the underscore as a suffix rather than a prefix. Any particular reason to go with the prefix here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, right you are; I'll update them for consistency

@Hamms Hamms force-pushed the maze-make-private branch 2 times, most recently from eead1e1 to ce2f94a Compare February 8, 2018 20:20
@Hamms Hamms changed the base branch from maze-to-class to staging February 8, 2018 20:20
@Hamms Hamms merged commit 8e8c8c7 into staging Feb 9, 2018
@Hamms Hamms deleted the maze-make-private branch May 31, 2018 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@balderdash balderdash balderdash approved these changes

@joshlory joshlory Awaiting requested review from joshlory

Assignees
No one assigned
Labels
code-hygiene cs-fundamentals
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Hamms @balderdash