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

Basic cube representation types #3

Merged
merged 3 commits into from Mar 13, 2020
Merged

Basic cube representation types #3

merged 3 commits into from Mar 13, 2020

Conversation

hoisinberg
Copy link
Collaborator

No description provided.

@KeijiBranshi
Copy link
Collaborator

All of these should probably just be .ts files. Only need .tsx when you're using React components

readonly column: number;
}

export namespace CubeIndex {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think namespaces are discouraged (or rather, not preferred) in TS/JS. Will need @itstang to fact check me on this, but for es6 the file itself is effectively the "namespace":
https://michelenasti.com/2019/01/23/is-typescript-namespace-feature-deprecated.html
microsoft/TypeScript#30994

Copy link
Collaborator

Choose a reason for hiding this comment

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

to keep the grouping you can also make it an object. Or, I see dimension as an argument for both functions. You could curry that out and make this a factory.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 would probably recommend using es6 standard modules over namespaces

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense. I've moved the static factory methods into a factory object.

export interface Cube {
readonly dimension: number;
readonly positionOf: (facelet: number) => CubeIndex;
readonly occupantOf: (position: CubeIndex) => number;
Copy link
Collaborator

Choose a reason for hiding this comment

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

idk if you want to add more context to these primitives (e.g. number). But if so, you can make type aliases:

type Facelet = number;
type Color = "red" | "white" | "blue" | "green" | "orange" | "yellow"

then reference the alias instead of number

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think he might want to abstract way from specific colors and just be able to use facelet indicies, in which case I don't think there's a simple way where we can enforce that type

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, the color was just meant to show what you can do with types. The Facelet = number is the more relevant example.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's neat. Done.

@hoisinberg hoisinberg merged commit 6d6ad96 into master Mar 13, 2020
@hoisinberg hoisinberg deleted the cuberep branch March 13, 2020 05:03
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.

None yet

3 participants